feat(agentex-ui): account picker via same-origin BFF proxy#350
Conversation
|
Addressed the Greptile findings in
Also, per UX design feedback, moved the account picker out of the header/collapsed-top rail into the sidebar footer (below Give Feedback) — top of the sidebar is untouched. |
Route the SDK through a same-origin BFF (`/api/agentex`) instead of calling the agentex API directly from the browser, and add an account switcher. - BFF proxy + shared `applyBffCredentials` forward the request cookies and an `x-selected-account-id` header to the upstream (agentex + platform APIs). - The selected account is driven by the `account_id` query param — no cookie — and injected on every SDK request via a synchronous ref. - `/api/user-info` fetches the caller's accounts; the picker bootstraps to the first account when the param is missing/stale (the API needs one to resolve a principal). Single account renders as static context; collapsed rail shows an icon-only variant. - Gated on the platform API being configured (`SGP_API_URL` / `NEXT_PUBLIC_SGP_APP_URL`); no-op otherwise. Env: `NEXT_PUBLIC_AGENTEX_API_BASE_URL` → server-only `AGENTEX_API_URL`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oter Address Greptile review + UX design feedback. - BFF: drop any client-supplied `Authorization` in applyBffCredentials so a client can't inject its own bearer token to the upstream (P1); strip `Location` from upstream 3xx responses so internal redirect targets don't leak to the browser (P2). - UX: move the account picker out of the sidebar header / collapsed top rail into the footer, directly below Give Feedback (top of the sidebar now untouched). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the account picker's loading skeleton with a disabled, empty picker (building icon only) — a plain skeleton block read as odd once the picker moved into the footer between Give Feedback and Log out. Keeps the row stable and reads as "loading account selector". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…switch - Account picker: extract a shared `iconTile` (loading + single-account collapsed tiles) and a shared `options` element (the SelectContent list previously duplicated across the collapsed and expanded switchers); single change handler `onAccountChange`. - Provider: `setSelectedAccountId` now clears `task_id` on an explicit switch (the open task is account-scoped and 404s under a new account). This preserves the reset that previously lived in agentex-ui-root's validation effect, which #347 reworked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The STRIP_REQ comment previously read as if `authorization` went unmanaged. Spell out that applyBffCredentials deletes any client-supplied `cookie`/`authorization` and sets the server-managed values, so the stripping applies to every /api/* proxy — addressing the review note without moving credential logic out of the shared helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8eb5b47 to
5f243a0
Compare
|
Will we need to update the sgp repo to pass in the |
yup working on the corresponding sgp PR, and will hold off on merging until thats ready |
…e flash Switching accounts closes the open task (task_id cleared), which flips PrimaryContent from ChatView to HomeView immediately. HomeView rendered the agents list from the still-cached previous account (invalidateQueries keeps data during the refetch), so the new account briefly flashed the old account's agents before recalibrating. Use resetQueries instead: the previous account's agents/tasks are dropped on switch, so HomeView shows a loading state until the new account's data lands — never stale data. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Trim redundant/verbose comments across the account-picker BFF layer (drop lines that restate the code or repeat a docstring); no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…itch A selected agent is account-scoped, but its name persisted in the URL across a switch — so a same-named agent in the new account stayed selected (and #347's localStorage restore could reinstate it). On an account change, clear agent_name and the remembered agent so the view resets to the grid. task_id is already cleared by the switch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7530498 to
54e5b75
Compare
Cookie-auth backends (SGP's identity-service-auth) resolve the account from the `_jwt` access-profile cookie when it's present and only fall back to `x-selected-account-id` when it's absent — so forwarding `_jwt` pins requests to the account the user linked in with and silently ignores the account they switched to. Drop `_jwt` before forwarding (default/cookie mode) so the selected account wins; identity stays via `_identityJwt`. agentex is unaffected (it never reads `_jwt`), and auth mode drops all cookies anyway. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| // doesn't block the localStorage-restore path when there's no agent_name. Deps are | ||
| // intentionally narrowed so we re-validate on fetch settle / agent_name change. | ||
| useEffect(() => { | ||
| // An account switch resets to the home grid: the selected agent is account-scoped, | ||
| // so a same-named agent in the new account must not stay selected. | ||
| if (accountRef.current !== sgpAccountID) { | ||
| accountRef.current = sgpAccountID; | ||
| setLocalAgentName(undefined); | ||
| if (agentName) updateParams({ [SearchParamKey.AGENT_NAME]: null }); | ||
| return; | ||
| } | ||
|
|
||
| if (isAgentsFetching || isAgentByNameFetching) return; |
There was a problem hiding this comment.
Bootstrap mistaken for account switch, permanently clears stored agent name
When a user loads the page without ?account_id in the URL, accountRef.current is initialized to null. The AccountPicker effect bootstraps the first account (via router.replace), changing sgpAccountID from null → 'first-account-id'. On the next render, this effect fires because sgpAccountID is in its deps, sees null !== 'first-account-id', and treats it as an explicit account switch — calling setLocalAgentName(undefined) (which overwrites localStorage) and returning before the restore-from-localStorage logic can run. Every fresh navigation to / without ?account_id permanently erases the user's saved agent preference. Single-account users (the most common case) are affected on every new tab or bookmarked-root visit.
A simple guard fixes it: skip the clear when accountRef.current is null (first load, no prior account) and only apply it when there is a known previous account being replaced.
Prompt To Fix With AI
This is a comment left during a code review.
Path: agentex-ui/components/agentex-ui-root.tsx
Line: 41-53
Comment:
**Bootstrap mistaken for account switch, permanently clears stored agent name**
When a user loads the page without `?account_id` in the URL, `accountRef.current` is initialized to `null`. The `AccountPicker` effect bootstraps the first account (via `router.replace`), changing `sgpAccountID` from `null` → `'first-account-id'`. On the next render, this effect fires because `sgpAccountID` is in its deps, sees `null !== 'first-account-id'`, and treats it as an explicit account switch — calling `setLocalAgentName(undefined)` (which overwrites localStorage) and returning before the restore-from-localStorage logic can run. Every fresh navigation to `/` without `?account_id` permanently erases the user's saved agent preference. Single-account users (the most common case) are affected on every new tab or bookmarked-root visit.
A simple guard fixes it: skip the clear when `accountRef.current` is `null` (first load, no prior account) and only apply it when there is a known previous account being replaced.
How can I resolve this? If you propose a fix, please make it concise.## Summary Second of a **2-PR stack**. Stacked on #350 — **base is `feat/agentex-ui-account-picker`**, so this diff is auth-only (GitHub auto-retargets to `main` once #350 merges). The chart + deploy wiring lives in scaleapi/sgp#3961. Adds opt-in OIDC login on top of the BFF, keeping the access token out of the browser entirely. - NextAuth (generic OIDC provider) selected **and** enabled by a single env var, `AGENTEX_UI_AUTH_PROVIDER_ID`; disabled by default so non-auth deployments are unaffected (no `SessionProvider`, no `/api/auth/session` calls). - The BFF attaches the access token as a `Bearer` server-side via `getToken` and drops the UI session cookie — the token never reaches client JS or the NextAuth session (which exposes only `error`). - Token/end-session endpoints are resolved from the issuer's **OIDC discovery** document (cached; failures retried), so refresh and logout work for any compliant IdP — no hardcoded provider paths. - Middleware auto-redirects unauthenticated requests to sign-in; a client guard re-auths on refresh failure; **POST-only** RP-initiated logout ends the IdP session (a GET would be cross-site-triggerable — logout CSRF). - Supports `client_secret_post` (dev) and `private_key_jwt` (prod). ## Why token-hidden (BFF) over exposing it on the session Exposing the access token to client JS is contrary to the OAuth 2.0 for Browser-Based Apps BCP and would flag in security review. Keeping it server-side (the BFF attaches the Bearer) is the recommended pattern; the client only ever sees the same-origin proxy. ## Stack 1. #350 — account picker + BFF proxy 2. **This PR** — OIDC login → stacked on #350 3. scaleapi/sgp#3961 — Helm chart wires the OneAuth OIDC client + env (`AGENTEX_UI_AUTH_PROVIDER_ID`, `OIDC_ISSUER_URL`, the client Secret via `envFrom`) ## Test plan - [x] `npm run typecheck` / `npm run lint` — clean - [x] Runtime login smoke test (sign-in → agents load via Bearer → logout ends the IdP session) - [x] Chart wiring — done in scaleapi/sgp#3961 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR adds opt-in server-side OIDC authentication to agentex-ui via NextAuth v5. When `AGENTEX_UI_AUTH_PROVIDER_ID` is set, the middleware gates all page routes, the BFF attaches the access token as a `Bearer` header server-side (keeping it out of client JS), and a `LogoutButton` drives POST-only RP-initiated logout. - **OIDC discovery-based endpoint resolution** (`discoverOidc`) replaces the previously hardcoded Ory paths for both `token_endpoint` (refresh) and `end_session_endpoint` (logout), directly addressing the concerns raised in the last review cycle. - **Token refresh error handling** treats all 4xx (except 429) as terminal `RefreshAccessTokenError`, clearing the stale token and triggering `SessionGuard` to force re-auth — the prior `invalid_grant`-only terminal set has been broadened. - **Logout CSRF** is prevented by making `/api/auth/logout` POST-only, with `SameSite=Lax` cookies not forwarded on cross-site POSTs. <details><summary><h3>Confidence Score: 5/5</h3></summary> Safe to merge — all three blocking concerns from the previous review cycle are addressed; no new correctness issues were found. The auth flow is well-layered: discovery-backed endpoint resolution, POST-only logout with RP-initiated IdP session termination, and a broadened 4xx terminal set covering invalid_client and other non-retryable errors. The two remaining notes (indefinite discovery cache TTL and silent local-only logout when id_token is absent) are edge cases that do not affect correctness for a standard OIDC-compliant IdP. No files require special attention. auth.ts is the most complex file but is well-commented and structurally sound. </details> <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | agentex-ui/auth.ts | New 327-line NextAuth v5 OIDC config with PKCE, private_key_jwt, discovery-based endpoint resolution, and JWT refresh. Discovery caching and startup validation are well-structured; P2: success path caches the discovery doc indefinitely with no TTL. | | agentex-ui/app/api/auth/logout/route.ts | POST-only RP-initiated logout using OIDC discovery for the end_session_endpoint. Properly addresses the CSRF and hardcoded-endpoint issues from the previous review cycle. | | agentex-ui/middleware.ts | Auth middleware that gates all page routes (not /api/) behind NextAuth; gated behind authEnabled so non-auth deployments are unaffected. | | agentex-ui/app/api/auth/auto-signin/route.ts | Server-side OIDC redirect initiation with open-redirect guard on redirect_url; correctly skips redundant round-trip for already-authenticated sessions. | | agentex-ui/app/api/_lib/bff.ts | BFF credential attachment updated to forward Bearer token in auth mode vs. cookies in default mode; correctly drops cookies when auth is enabled. | | agentex-ui/components/providers/agentex-provider.tsx | Adds 401 retry with deduplicated session refresh; the module-level promise dedup correctly handles concurrent 401s from a refocused tab. | | agentex-ui/components/session-guard.tsx | Client-side re-auth trigger on RefreshAccessTokenError; complements the middleware redirect for API-only sessions like an active chat. | | agentex-ui/components/task-sidebar/task-sidebar-footer.tsx | Adds LogoutButton gated by authEnabled; uses POST for logout to prevent CSRF; follows the server-provided RP-initiated logout URL correctly. | | agentex-ui/app/layout.tsx | Conditionally mounts SessionProvider with refetchInterval=240 when auth is enabled; re-derives authEnabled from env instead of importing from @/auth (addressed in prior review). | | agentex-ui/app/login/page.tsx | Simple redirect shim forwarding /login?redirect_url=... to auto-signin; open-redirect validation happens in auto-signin. | </details> <details><summary><h3>Sequence Diagram</h3></summary> <a href="#gh-light-mode-only"> ```mermaid %%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Browser participant Middleware participant AutoSignin as /api/auth/auto-signin participant NextAuth as /api/auth/[...nextauth] participant IdP as OIDC IdP participant BFF as /api/agentex (BFF) participant Upstream as Upstream API Browser->>Middleware: GET /some-page (no session) Middleware->>AutoSignin: "redirect to auto-signin?redirect_url=/some-page" AutoSignin->>NextAuth: signIn(providerId, redirectTo) NextAuth->>Browser: 302 to IdP /authorize (PKCE + state) Browser->>IdP: follow OIDC auth redirect IdP->>NextAuth: callback to /api/auth/callback/id NextAuth->>NextAuth: jwt callback stores access_token (never on session) NextAuth->>Browser: set encrypted session cookie, redirect to /some-page Note over Browser,Middleware: Authenticated flow Browser->>BFF: fetch /api/agentex/... (session cookie) BFF->>BFF: getSessionToken(req) extracts accessToken BFF->>Upstream: GET ... Authorization: Bearer accessToken Upstream->>BFF: 200 OK BFF->>Browser: 200 OK Note over Browser,IdP: Logout flow Browser->>Browser: POST /api/auth/logout Browser->>NextAuth: signOut clears session cookie Browser->>IdP: "GET end_session_endpoint?id_token_hint=..." IdP->>Browser: redirect to post_logout_redirect_uri Note over Browser,NextAuth: Token refresh every 240s via SessionProvider Browser->>NextAuth: GET /api/auth/session NextAuth->>IdP: POST token_endpoint (refresh_token grant) IdP->>NextAuth: new access_token + refresh_token NextAuth->>Browser: rotated session cookie ``` </a> <a href="#gh-dark-mode-only"> ```mermaid %%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Browser participant Middleware participant AutoSignin as /api/auth/auto-signin participant NextAuth as /api/auth/[...nextauth] participant IdP as OIDC IdP participant BFF as /api/agentex (BFF) participant Upstream as Upstream API Browser->>Middleware: GET /some-page (no session) Middleware->>AutoSignin: "redirect to auto-signin?redirect_url=/some-page" AutoSignin->>NextAuth: signIn(providerId, redirectTo) NextAuth->>Browser: 302 to IdP /authorize (PKCE + state) Browser->>IdP: follow OIDC auth redirect IdP->>NextAuth: callback to /api/auth/callback/id NextAuth->>NextAuth: jwt callback stores access_token (never on session) NextAuth->>Browser: set encrypted session cookie, redirect to /some-page Note over Browser,Middleware: Authenticated flow Browser->>BFF: fetch /api/agentex/... (session cookie) BFF->>BFF: getSessionToken(req) extracts accessToken BFF->>Upstream: GET ... Authorization: Bearer accessToken Upstream->>BFF: 200 OK BFF->>Browser: 200 OK Note over Browser,IdP: Logout flow Browser->>Browser: POST /api/auth/logout Browser->>NextAuth: signOut clears session cookie Browser->>IdP: "GET end_session_endpoint?id_token_hint=..." IdP->>Browser: redirect to post_logout_redirect_uri Note over Browser,NextAuth: Token refresh every 240s via SessionProvider Browser->>NextAuth: GET /api/auth/session NextAuth->>IdP: POST token_endpoint (refresh_token grant) IdP->>NextAuth: new access_token + refresh_token NextAuth->>Browser: rotated session cookie ``` </a> </details> <sub>Reviews (10): Last reviewed commit: ["fix(agentex-ui): escalate refresh failur..."](56998b0) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=42798531)</sub> <!-- /greptile_comment --> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
First of a 2-PR stack bringing account scoping + OIDC login to agentex-ui. This base PR routes the SDK through a same-origin BFF and adds an account switcher. Chart + deploy side lives in scaleapi/sgp#3961.
/api/agentex) instead of calling the API directly from the browser — the upstream URL and credentials never reach client JS.applyBffCredentialsforwardsx-selected-account-id, drops any client-sentAuthorization, and strips the account-scoped_jwtcookie so SGP honors the selected account rather than the one you linked in with (identity stays via_identityJwt). The proxy also stripsLocationon upstream 3xx so internal redirect targets don't leak.account_idquery param (no cookie), injected on every SDK request via a synchronous ref. Switching accounts resets the open task + selected agent to the account's home grid, andresetQueriesdrops the previous account's cached data so it never briefly renders the old agents./api/user-infofetches the caller's accounts; the picker bootstraps to the first when the param is missing/stale, shows a disabled/empty loading state (not a skeleton), renders single-account as static context, and lives in the sidebar footer (collapsed → icon-only).SGP_API_URL/NEXT_PUBLIC_SGP_APP_URL); no-op otherwise.Env:
NEXT_PUBLIC_AGENTEX_API_BASE_URL→ server-onlyAGENTEX_API_URL.In this PR the BFF runs in default (cookie) mode — no auth dependency. The token-hiding Bearer path arrives in the stacked PR.
Stack
mainTest plan
npm run typecheck/npm run lint— clean_jwtstrip); single / multi / no-account render; loading state; no stale-account flash🤖 Generated with Claude Code
Greptile Summary
This PR routes the Agentex SDK through a same-origin BFF proxy (
/api/agentex) so the upstream URL and credentials stay server-side, and adds an account picker driven by theaccount_idquery param. A new sharedapplyBffCredentialshelper manages cookie stripping (_jwt) and header hygiene across the BFF routes; a scoped/api/user-infoendpoint bootstraps and switches accounts without exposing arbitrary platform endpoints.app/api/agentex/[...path]/route.ts): streams all HTTP methods through to the upstream, strips hop-by-hop and credential headers, and dropslocationon upstream 3xx so internal redirect targets don't escape.components/account-picker/account-picker.tsx): bootstraps to the first account whenaccount_idis absent, resets account-scoped React Query cache on switch (preservinguser-info), and renders as an icon-only tile in the collapsed sidebar rail.components/providers/agentex-provider.tsx): injectsx-selected-account-idon every SDK request via a synchronous ref to avoid a race between URL navigation and the next fetch.Confidence Score: 4/5
Safe to merge after fixing the bootstrap/localAgentName regression in agentex-ui-root.tsx.
The account-switch detection in
agentex-ui-root.tsxtreats the initial bootstrap (null → first account on a fresh URL load) the same as an explicit switch, causingsetLocalAgentName(undefined)to permanently erase the user's stored agent preference on every fresh-tab or direct-URL visit. Users with a single account — the most common deployment — would lose their last-used agent selection on every new session. The BFF proxy, credential stripping, and account picker logic are otherwise well-structured.agentex-ui/components/agentex-ui-root.tsx — the bootstrap path needs to be distinguished from an explicit account switch before the localStorage clear runs.
Important Files Changed
_jwtcookie, deletesauthorization, forwardsx-selected-account-id. Logic is correct;asyncsignature is forward-compatible with the stacked PR's Bearer path.applyBffCredentials, dropslocationon upstream 3xx. Streaming via unbuffered body is correct. Previousauthorizationandlocationissues from prior review round are fixed.accountRef. The bootstrap (null → first account) is indistinguishable from an explicit switch, causinglocalAgentNameto be permanently cleared and the restore-from-localStorage path to be skipped on every fresh-URL page load.useMemo([])with ref access is the correct pattern here.resetAccountScopedcorrectly preserves user-info while clearing other account-scoped query cache.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Browser participant BFF as Next.js BFF<br/>(same-origin) participant Agentex as Agentex API participant SGP as SGP Platform API Note over Browser,SGP: Account bootstrap (first load, no account_id in URL) Browser->>BFF: GET /api/user-info (cookies) BFF->>SGP: GET /user-info (cookies minus _jwt) SGP-->>BFF: "{ access_profiles: [...] }" BFF-->>Browser: "{ access_profiles: [...] }" Browser->>Browser: "router.replace(?account_id=first)" Note over Browser,Agentex: SDK request with selected account Browser->>BFF: GET /api/agentex/v1/agents x-selected-account-id: abc BFF->>BFF: applyBffCredentials() drop authorization strip _jwt cookie forward x-selected-account-id BFF->>Agentex: GET /v1/agents x-selected-account-id: abc Agentex-->>BFF: 200 agents[] BFF-->>Browser: 200 agents[] (streamed) Note over Browser,SGP: Account switch Browser->>Browser: "onAccountChange resetQueries router.push(?account_id=new-id)" Browser->>BFF: GET /api/agentex/v1/agents x-selected-account-id: new-id BFF->>Agentex: GET /v1/agents x-selected-account-id: new-id Agentex-->>BFF: 200 agents[] (new account) BFF-->>Browser: 200 agents[]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Browser participant BFF as Next.js BFF<br/>(same-origin) participant Agentex as Agentex API participant SGP as SGP Platform API Note over Browser,SGP: Account bootstrap (first load, no account_id in URL) Browser->>BFF: GET /api/user-info (cookies) BFF->>SGP: GET /user-info (cookies minus _jwt) SGP-->>BFF: "{ access_profiles: [...] }" BFF-->>Browser: "{ access_profiles: [...] }" Browser->>Browser: "router.replace(?account_id=first)" Note over Browser,Agentex: SDK request with selected account Browser->>BFF: GET /api/agentex/v1/agents x-selected-account-id: abc BFF->>BFF: applyBffCredentials() drop authorization strip _jwt cookie forward x-selected-account-id BFF->>Agentex: GET /v1/agents x-selected-account-id: abc Agentex-->>BFF: 200 agents[] BFF-->>Browser: 200 agents[] (streamed) Note over Browser,SGP: Account switch Browser->>Browser: "onAccountChange resetQueries router.push(?account_id=new-id)" Browser->>BFF: GET /api/agentex/v1/agents x-selected-account-id: new-id BFF->>Agentex: GET /v1/agents x-selected-account-id: new-id Agentex-->>BFF: 200 agents[] (new account) BFF-->>Browser: 200 agents[]Prompt To Fix All With AI
Reviews (9): Last reviewed commit: "Merge branch 'main' into feat/agentex-ui..." | Re-trigger Greptile