Repository navigation
🛂 fix: Decide Admin Access Before the Panel Renders and Fail Closed on Errors - #185
Conversation
…n Errors The /_app layout read the viewer's capabilities with a component query and showed AccessDenied only once that query had succeeded. Until then it rendered the whole panel: sidebar, header and the routed page. A signed-in user without access:admin, such as one whose grant was revoked within the 60-second session revalidation window, saw the panel before the denial. When the capabilities request failed with an error the hook does not map, such as a 500, the panel stayed up for them. The /_app loader now loads the capabilities through capabilitiesQueryOptions(userId), the query useCapabilities reads, so the layout first renders with the answer. An admin gets the panel, anyone else gets AccessDenied, and a failed load shows PermissionsUnavailable with its Reload button. The AppPending shell covers the wait. A failed background refetch keeps the last answer, so an admin stays in the panel; a refetch without access:admin still takes the panel away. Admins also get their full sidebar on first paint instead of Configuration and Access appearing after load. The unused effectiveCapabilitiesOptions, which cached a different shape under the same query key, is removed. Supersedes #109 and #101 by @dustinhealy: the loader approach comes from #109 and the fail-closed states from #101.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 373f1538de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e9c5e66e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…kend (#190) Since #185 the /_app route loads GET /api/admin/grants/effective before the panel renders, and fails closed when it can't. e2e/mock-backend.mjs never served that route, so every signed-in page in the mock e2e run showed "Could not verify permissions". The mock now answers it for its admin tokens with every capability in SystemCapabilities, refusing other requests the way it refuses /api/admin/verify; both share one token check.
…k and Update the Access and Grants Specs (#202) * 🧪 fix: Serve the Admin's Effective Capabilities from the E2E Mock Backend Since #185 the /_app route loads GET /api/admin/grants/effective before the panel renders, and fails closed when it can't. e2e/mock-backend.mjs never served that route, so every signed-in page in the mock e2e run showed "Could not verify permissions". The mock now answers it for its admin tokens with every capability in SystemCapabilities, refusing other requests the way it refuses /api/admin/verify; both share one token check. * 🧪 fix: Serve Roles, Groups, Grants and the Audit Log from the E2E Mock and Update the Access and Grants Specs e2e/mock-backend.mjs served none of the admin API routes the Access and Grants pages read, so most of access.spec.ts and grants.spec.ts failed against it. The mock now holds in-memory fixtures (users, the ADMIN and USER roles plus Auditor, Billing and Support, the Engineering and Marketing groups, role grants and a hash-chained grant history) and answers the roles, groups, user search, grants and audit-log routes in LibreChat's response shapes. Group search is LibreChat's case-insensitive substring match on name, email and description; user search matches word prefixes. Writes change the fixtures, so create, edit and delete flows work. Path parameters are matched per route and every admin route shares the existing token check. The specs also asserted UI the panel does not have: title-cased role names, a searchbox role, kebab menus, keyboard reordering, a roles/groups type filter and user grants. They now follow the current panel, wait for filtered results instead of sleeping, and delete the roles and groups they create. Under E2E_LIVE=1 the Groups-tab tests create the groups they list and delete them afterwards, and the tests that read users or grant history only the mock has are skipped. * 🧪 fix: Name Live E2E Roles and Groups With UUIDs, Serve the Audit-Log CSV Export From the Mock and Test That a Role Rename Leaves Its Grants Behind * 🧪 fix: Clean Up Live E2E Staging on Failure, Keep Audited Writes on the Mock and Find Staged Groups Past the First Page * 🧪 fix: Serve One Audit-Log Entry by Id From the E2E Mock
Pull Request Template
Summary
This PR supersedes #109 and #101, both by @dustinhealy, and combines them on current
main. The loader approach comes from #109 and the fail-closed states come from #101. Neither PR applies as it stands: #109 conflicts withmain, and #101'sreturn nullwhile loading would blank the page wheremainnow shows theAppPendingshell.The
/_applayout decided admin access inside the component, after the page had already rendered:This caused two problems for a signed-in user without
access:admin, for example a user whose grant was revoked within the 60-second session revalidation window:/api/admin/grants/effectivewas in flight, the layout rendered the whole panel (sidebar, header, dashboard with its Quick links) and only then switched to Access denied.useCapabilitiesdoes not map (anything outside 401/403/404/503, such as a 500),!isErrorkept the guard from ever tripping. The panel stayed up for that user, on every page.Admins had a smaller version of the same problem: their sidebar first rendered without Configuration and Access, and those items appeared once the capabilities arrived.
Changes
useCapabilities.tsexportscapabilitiesQueryOptions(userId), so the loader and the hook share one query definition and one cache entry. It keeps the existing 401/403 → no capabilities and 404/503/fetch failed→ grants unavailable mapping,staleTime30s andretry: false.useCapabilities()uses it, and its signature is unchanged. The{ available, capabilities }shape is nowt.CapabilitiesDatainsrc/types/hooks.ts./_approute loads the capabilities in itsloaderwithfetchQuery, afterbeforeLoadhas verified the session. A cached answer is used only while fresh (30 seconds); a stale one is fetched again before the layout renders, so returning to the panel after a revocation doesn't flash it either. If that fetch fails, the loader reports the capabilities as unverified and the layout shows "Could not verify permissions" instead of trusting the stale answer. Until the capabilities are known, the router shows the existingAppPendingshell (pendingMs300), so the layout first renders with the answer.AppLayoutrenders the panel only when the grants API is available and the capabilities implyaccess:admin. Everyone else getsAccessDenied, before any sidebar, header or page mounts.AppLayoutthen showsPermissionsUnavailablewith its Reload button, not the panel and not the generic error page. The query is not retried on mount, because the loader has just tried it.EditCapabilitiesDialoginvalidates['effectiveCapabilities']after a save, and the window-focus refetch runs once the query is stale. A failed refetch therefore cannot take an admin out of the panel or into the error page. A refetch that comes back withoutaccess:adminstill replaces the panel with Access denied.effectiveCapabilitiesOptionsfromsrc/server/capabilities.ts. Nothing called it, and it cached astring[]under the same['effectiveCapabilities', userId]key the hook stores{ available, capabilities }under.Not carried over
{ queryClient: QueryClient }root context typing and the loading-state pending component.mainalready hascreateRootRouteWithContext<t.RouterContext>()and theAppPendingshell.return nullwhile loading. TheAppPendingshell stays.access.tsx,configuration/index.tsx) are unchanged.Change Type
Testing
New unit tests in
src/routes/-tests/app.test.tsxrender the real/_approute and dashboard under a memory router. AMutationObserverrecords everyaside,headerandmainelement that enters the document, including one that a later render removes again:AppPendingshell is shown, with no chrome.access:admin, a 403, or no grants API (404) each show Access denied, and no chrome mounts first.access:adminreplaces the panel with Access denied.On
main, the pending, three denied, unavailable and allowed tests fail. The two refetch tests pass on both; they guard the fix against a version that would throw admins out on a failed refetch.Live check against a LibreChat backend and the production build. The test user's only admin access was a user-level
access:admingrant. The grant was revoked, and the page was reloaded within the 60-second revalidation window. AMutationObserverinstalled before the app's scripts recorded when each element first appeared:main/grants)/grants)Dashboard, Grants, Help, then Configuration and Access 14–39 ms laterTest Configuration:
devbackend, MongoDB 8.2 replica set.bun run build+bun server.ts), Chromium via Playwright.npx tsc --noEmit,npx eslint src/ e2e/,npx prettier --checkon the changed files,bun run test:lint-rules(19 pass),npx vitest run(153 files, 3323 tests).Checklist