feat(webapp): impersonation consent page and a view-as-user toggle - #4421
Merged
Conversation
Opening a `/@/orgs/<slug>/…` link from outside the dashboard (address bar, bookmark, a link shared elsewhere) used to bounce back to /admin, because starting impersonation requires a same-origin navigation. Keep that requirement for the state change, but render a consent interstitial instead of blocking: the page names the organization and destination, and its "Impersonate" button posts back from our own page, so the same-origin check still holds. In-app admin links are unchanged and still impersonate in one click. Also add a display-only "View as user" toggle for impersonation sessions. It lives on the impersonation cookie, so it disappears when impersonation is cleared, and it hides admin-only UI both client-side (via useHasAdminAccess) and in the loaders that compute what to show. It never touches authorization. The escape hatches — the impersonation border, "Stop impersonating", the global shortcut and the toggle itself — stay visible while it's on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G5rpYGm4SVqxSNXx6sXtpu
|
…module The consent route exported `findImpersonationTarget` and `startImpersonation` alongside its loader and action. Remix only strips `loader`, `action` and `headers` from a route module when building the browser bundle, so those extra exports kept `~/db.server` in the client graph and the build failed with "Server-only module referenced by client". Move both helpers into `~/models/admin.server`, next to `redirectWithImpersonation` and `clearImpersonation`, and import them from the route. The route now only exports `loader`, `action` and its component, so every `.server` import it has is removed for the client.
…ser toggle The consent page's form had no `action`, so React Router resolved it to the matched route's `pathnameBase`. Without `future.v3_relativeSplatPath` that excludes the splat, so the form posted to `/@/orgs/<slug>`, the action saw an empty splat, and the admin landed on the organization root instead of the deep link the page had just named. The form now posts to an explicit absolute path. The query string was dropped too. A `/@/runs/<id>` link redirects to a run path carrying `?span=`, which selects the span to open, so both the displayed destination and the final redirect now carry the incoming search. The debug tooltip and the "Debug run" button still checked `!hasAdminAccess && !isImpersonating`, which stays true with the toggle on, so the two most visible admin-only affordances kept rendering. `useHasAdminAccess` already folds in impersonation and the toggle, so the extra check is gone. The replay dialog's region picker was likewise still keyed off raw impersonation and now uses `hasAdminDisplayAccess`. Also: gate the view-as-user route on a same-origin navigation like the other impersonation routes, stop it flat-route-nesting under `resources.impersonation` (the URL is unchanged), and log consent-page entry at info with only the referer's origin, since it is expected for any link opened outside the app.
…n mock The replay loader now derives the region picker's `isAdmin` from `hasAdminDisplayAccess`, but this file's `vi.mock` of `~/services/session.server` is a factory that replaces the whole module, and it only returned the three functions the route graph used before. Accessing an export a factory mock does not define throws, so every case in the file failed once the loader was reached: Error: [vitest] No "hasAdminDisplayAccess" export is defined on the "~/services/session.server" mock. Did you forget to return it from "vi.mock"? The stub mirrors the real predicate rather than importing it, since pulling the original module in would evaluate the server-only import graph this file deliberately keeps out. The fixture user gains `isViewingAsUser: false`, so the flag stays false exactly as it did before, and the presenter that reads it is mocked out anyway. Co-Authored-By: Claude <noreply@anthropic.com>
carderne
marked this pull request as ready for review
July 30, 2026 08:03
Member
|
It would be nice to also hide the yellow impersonation border:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… capability gates raw The yellow side-menu border is the loudest tell that you are not the user, so "view as user" now hides it too — the point of the mode is a dashboard that looks exactly like the user's. The ways out stay on raw impersonation: "Stop impersonating" and the toggle itself in the account menu, the Cmd+Opt+A shortcut, and the routes that clear the impersonation cookie. The write suppressions (side-menu preferences, favorites) also stay raw — they keep an admin from writing into the impersonated account and are not display chrome. Two more review findings, same area: - The already-impersonating branch of the impersonation deep link kept only the pathname, so an admin who was already impersonating and opened a run link lost the ?span= selection and the follow-up GET built its destination and post-back paths from an empty search. Keep the search. - hasAdminDisplayAccess is display-only, but it had reached two gates that decide what a request may do: the default-region service call and the query page's EXPLAIN flag. With the toggle on, an admin's own form submissions were rejected. Both are back on raw user.admin || user.isImpersonating, and the helper now states the rule: the toggle changes what is shown, never what is permitted. Every loader and rendering site stays on the helper. Co-Authored-By: Claude <noreply@anthropic.com>
Two review findings, both about "who is this request" being answered differently in different places. The consent page's action went straight from requireUser to the admin check. requireUser resolves to the impersonated user whenever the impersonation cookie is set, so user.admin was false for a legitimate admin who already had an impersonation session: render the consent page with nothing active, start impersonating in another tab, then press Impersonate on the still-open page, and the POST redirected to / with no explanation. The action now mirrors the loader's ordering — clear the impersonation first and come back to the same URL (search included) under the admin's own identity, where the normal flow re-authorizes them and they can confirm. The impersonation mutation never runs on a request whose resolved identity is the impersonated user. The view-as-user flag had two definitions. requireUser required the impersonated id to match the resolved user; the root loader only required that some impersonated id was present in the session. Those disagree in exactly one state: getUserId deliberately falls back to the real admin's id when they are no longer an admin, so the cookie still names a target that is not the resolved user. The client-side flag read true while the server-computed user.isViewingAsUser read false, and client-hidden admin UI drifted from server-computed display flags. The strict rule now lives in one place, resolveImpersonationState, with getImpersonationState reading the cookie and delegating to it. requireUser, the root loader and the bulk-action loader all go through it, so the value published to the client is the value the server computed. A de-admined session correctly reports neither impersonating nor viewing as user, which also restores its admin chrome — right, since it is not an admin session any more. The organization loader's own isImpersonating is left as it was; it is pre-existing and nothing here depends on it. Co-Authored-By: Claude <noreply@anthropic.com>
…display one Four of the hasAdminDisplayAccess sites feed a region picker that submits, and an option list a form posts from is not display. Shrinking it changes what the submitted form is able to do, so an admin who flipped "view as user" on quietly lost the ability to send a run to a hidden or restricted region from the replay dialog, the test form, the playground and the bulk-action override. All four are back on raw user.admin || user.isImpersonating. The replay dialog is the only one where the list and the current value can disagree, because the run's own region is returned separately from the list. It degrades better than it looks: the Select's hidden native select synthesises an option for a value that is not among the registered items, so an out-of-list region is displayed and posted unchanged rather than falling back to the first option — and when the list drops to a single entry the field is not rendered at all, at which point the replay service falls back to the run's own region. Nothing was silently re-routed; the cost was the narrowed choice. The regions page listing stays on hasAdminDisplayAccess. It is a read-only table, and each row's "set as default" form posts that row's own id, so a shorter list can only offer fewer rows, never a wrong value — and the action behind it already re-checks raw impersonation. The helper's comment now draws the sharper line, since "display" was reading as broader than intended: display is rendering and read-only listings, while anything a request handler reads, or that feeds a control which submits, stays raw. The run-GET guard test's session mock drops the hasAdminDisplayAccess stub it needed only for the replay loader. No module in that test's graph reaches the export now, and the file still fails only on the container runtime it cannot reach in a sandbox, with no missing-export error. Co-Authored-By: Claude <noreply@anthropic.com>
It was rendered nowhere; the live impersonation affordances live in the side menu's account popover. Co-Authored-By: Claude <noreply@anthropic.com>
carderne
approved these changes
Jul 30, 2026
1stvamp
added a commit
that referenced
this pull request
Jul 31, 2026
) `test/envParamRoute.ownership.test.ts` fails on main: 3 of its 4 tests throw ``` Error: [vitest] No "hasAdminDisplayAccess" export is defined on the "~/services/session.server" mock. Did you forget to return it from "vi.mock"? ``` #4421 added a `hasAdminDisplayAccess(user)` call to the `env.$envParam` loader, and the test's `vi.mock` of `session.server` only returns `requireUser`, so the call blows up. Both changes were green in their own PR and only conflict once merged together, which is why nobody caught it. The mock now mirrors the real implementation rather than returning a constant, so it stays correct if the test's user fixture is ever varied. No assertions were changed: the tests were right, the mock was stale. Worth flagging separately: no workflow runs on push to main, so this has been red since #4421 landed without showing up anywhere. Every PR opened since has inherited the failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Chris Arderne, Eric Allam · Slack thread
Before / After
Impersonation links opened from outside the dashboard
/@/orgs/<slug>/…link into the address bar, opening it from a bookmark, or clicking it from a message got silently bounced to/admin. Starting impersonation requires a same-origin navigation (so a cross-site navigation can't silently start it), and those entries don't qualify — so the whole link-sharing workflow was broken.Seeing what the user sees while impersonating
How
The
/@/orgs/:slug/*route becomes a full route module (loader + action + UI). The loader keeps its existing branches in the same order and still starts impersonation immediately for a same-origin navigation; otherwise it does a read-only organization lookup and returns data for a consent interstitial (no cookie set, no audit row written). TheFormhas noaction, so it posts to the current URL and the org slug plus splat path reach the action unchanged; the action re-applies the same-origin check, rejects non-POST with 405, and calls the same shared helper the loader's fast path uses. The button uses theButtonprimitive'sshortcutprop, and the form usesreloadDocumentso every loader re-runs under the new cookie.The view-as-user flag is a key on the existing impersonation cookie session, which is why it's automatically scoped to the impersonation session —
clearImpersonationIdunsets it alongside the impersonated user id. It's exposed on the root loader payload and onrequireUser, read on the client through a newuseIsViewingAsUser()hook, and folded intouseHasAdminAccess()— which covers roughly twenty existing call sites at once.SideMenu's three direct admin reads now go through the same derived value. Server-side, a newhasAdminDisplayAccess(user)helper insession.server.tsreplaces the ad-hocuser.admin || user.isImpersonatingdisplay derivations in the affected loaders; it is documented as display-only and no authorization gate was touched (getUserId,requireSuper/canSuper, the query access gate, the logs-page gate and the/adminroutes are unchanged). A newPOST /resources/impersonation/view-asresource route flips the flag, requires an authenticated user, no-ops when the caller isn't impersonating, and redirects with a full navigation.Testing
apps/webapp/test/impersonationConsent.test.tslocks that resolving who a link would impersonate is read-only (no audit row) while the explicit POST does start impersonation, sets the cookie and preserves the destination path; plus the org-not-found / no-confirmed-member / deleted-org cases.apps/webapp/test/viewAsUser.test.tscovers the cookie helpers (on, off, never on without an impersonated user, dropped when impersonation is cleared) andhasAdminDisplayAccess. Testcontainers only, nothing mocked.pnpm run typecheck --filter webappclean;pnpm run formatandpnpm run lintclean.Changelog
Admins opening an impersonation link from outside the dashboard now get a confirmation page instead of being bounced, and while impersonating they can switch to "View as user" to see the dashboard without any admin-only UI.