Admin section under /admin, post-login deep-link fix, and common error pages - #274
Merged
Conversation
Deploying simple-module-python with |
| Latest commit: | 6459c86 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://280367e5.simple-module-python.pages.dev |
| Branch Preview URL: | https://feat-admin-section.simple-module-python.pages.dev |
AuthMiddleware stashed the URL an anonymous visitor asked for in session["next"], but the local provider never read it back: get_login_url() accepted a next_url argument and ignored it, and Login.tsx looked for a ?next= query param that nothing ever set. Every login landed on the configured default instead of the requested page. The Keycloak provider already consumed the session key correctly, so the two providers had silently drifted apart. Middleware now hands the provider a sanitised relative target and the local login view surfaces it as login_redirect_url — read, not popped, so reloading the login page does not downgrade the deep link. The POST handlers clear it once login actually succeeds. Login.tsx no longer reads ?next= from the query string. That was an open redirect: a crafted /users/login?next=https://evil.example would bounce the user off-site immediately after signing in. safe_next() moves from site_lock into simple_module_core.redirect_safety alongside the session-key constant, so the middleware, both providers and the site gate share one implementation rather than four copies of the same URL-safety rules. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
The Inertia error page only had copy for 403/404/500, so a 401, 429 or 503 rendered as a bare "Error / An unexpected error occurred" — telling the user nothing they could act on. Adds 401, 419, 422, 429 and 503, and collapses the three parallel Record<number, …> maps into one row per status, since maps that must be edited in lockstep drift. 401 and 419 now offer "sign in" as the primary action, since that is the actual remedy. The URL comes from the auth provider via app.state rather than an import — framework code must not reach into modules (SM009) — and an app with no provider installed simply gets no button. A test parses the status table out of Error.tsx and asserts every status the handler renders has copy for it. The two lists are in different languages and nothing else keeps them honest. Maintenance mode is a DB-backed HostSettings flag so flipping it does not need a redeploy, which is when you least want one. Admins pass through — someone must be able to reach settings and switch it off — and the auth provider's own routes stay open so an admin who was signed out when it flipped can still sign in. It fails open on missing config: a config gap taking the site down is the exact failure this feature would otherwise cause. Also fixes render_error_page reading app.state.sm outside its own try, which meant the documented JSON fallback never ran for that failure and the error page raised while reporting an error. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
The admin surfaces were scattered across the URL space — /settings/, /audit_log/, /users/admin, /dashboard/doctor — with nothing marking them as a section. MenuSection.ADMIN_SIDEBAR and AdminLayout already existed but only one screen (Doctor) ever used them, so the whole admin area was scaffolding with nothing in it. Every admin screen now lives under /admin/, registers into ADMIN_SIDEBAR, and renders in AdminLayout. Grouped Access / Appearance / System, which is what group= is for now that the admin sidebar is itself the separation — it previously tried to carve an admin area out of the main sidebar with ad-hoc "Administration" and "System" labels. Pure-admin modules just point view_prefix at /admin/...; that does not work for users and dashboard, which serve public and admin pages from one package. A module gets exactly one view router, so ModuleMeta gains an optional admin_view_prefix and ModuleBase an optional register_admin_routes hook, letting users keep /users/login while its CRUD moves to /admin/users. Both are additive and default to no-op, so no existing module changes. /admin itself is a host route: no module owns the admin section, it is assembled from whatever is installed, and a module could only serve it by claiming /admin as its own view_prefix. The page lists no tools of its own — it reads the adminSidebar shared prop, already filtered by the viewer's roles and permissions, so a card cannot advertise a screen its owner cannot open. Old URLs 301 to the new ones. Only view URLs moved; /api/* is a separate contract with external callers and is untouched. Two things found while moving: - settings' own /modules -> / redirect hardcoded the old path, so it would have sent users to a URL that no longer exists. Now derived from VIEW_PREFIX. - The bare-prefix route clone only ever covered routes declared directly on a router. include_router stores a placeholder and flattens later, so for the majority of modules it silently matched nothing. Documented rather than left implying coverage it lacks; the affected menu URLs use the canonical trailing-slash path, which is what every other module already did. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
The 503 rendered "Service unavailable" — the generic incident wording — even when an operator had deliberately taken the site down, and the maintenance_title/description strings added alongside the feature were unreachable: nothing ever selected them. MaintenanceMiddleware now marks the request, so the page can tell a planned outage from the same status arriving unbidden. That matters most when the operator sets no message, which is the case where the page has nothing else to say. Adds a test that every host.error.* key is referenced by Error.tsx. Copy that no code path can reach is invisible in review and in the running app — the page just quietly falls back to the generic message, which is exactly how these two keys shipped dead. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
Documents where admin screens go and why all three parts move together — URL, menu section, layout — since changing one leaves a page whose sidebar no longer lists it, and nothing about that failure is obvious from the diff. Living reference docs now point at the moved URLs. Dated plans, specs, release notes and perf runs are left alone: they record what was true when they were written. Two of the corrected links were already stale before this move — /settings/permissions and /settings/feature-flags never existed under the settings module. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
Ten findings from the Stage A review, most of them call sites the URL move missed because they were neither <Link href> nor router.get: - permissions RoleEdit/UserEdit PUT, feature_flags toggle/clear POST and audit_log filter navigation still targeted the pre-move prefixes. The legacy redirects are GET-only, so saving role permissions or toggling a flag would have 404/405'd and silently failed to persist. - /admin/doctor had no authorization dependency at all: any signed-in account could read migration status, module list and system info, and the Doctor menu item carried no roles= either, so it was visible to them too. Now guarded, gated in the menu, and covered by a regression test — this PR's own module-authoring guidance says to do both. - MaintenanceMiddleware consulted only the provider's legacy get_public_paths(), not the PublicRouteRegistry that AuthMiddleware also checks. Branding's logo/favicon are registered there precisely so the sign-in page can render the brand, so the maintenance page was losing its own branding — and any module's deliberately-public route (webhooks, STAC/OGC reads) was blocked. Fails closed when no registry is present. - The legacy redirect dropped the query string, so a bookmarked filtered or tabbed URL reset itself on the way through. - permissions' post-save redirect and a RoleEdit cancel link used bare or hardcoded paths instead of the canonical trailing slash and the USERS_ADMIN_PATH constant. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
Follow-up to review finding #11. The reviewer flagged the hardcoded is_admin() gate as a design call; the narrower problem underneath it is a real inconsistency. The overview renders from the adminSidebar shared prop, which the menu registry filters by roles AND permissions. Admission checked the admin role alone. So a custom role holding, say, settings.view could open /admin/settings/, see the AdminLayout badge pointing at /admin, click it, and get a 403 — from a page whose whole job was to list the one tool they can use. Admission now mirrors what the page renders, read from the shared props the layout middleware already built for the request rather than recomputing the filter (recomputing is how the two drift apart). Admins are still admitted unconditionally so an install with no admin modules reaches the page's own empty state instead of a 403, which would have made that copy unreachable. Whether "admin" should become a real permission is left alone — that is the architecture question the reviewer raised, and it is not this branch. Adds host/tests/, which needed a fixture that mounts the host router: the framework app fixture builds create_app() only, so every host-level route was previously untested and /admin simply 404'd. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
audit_log/feature_flags/settings menu tables still said Section=SIDEBAR after their module.py registrations moved to MenuSection.ADMIN_SIDEBAR; feature_flags/users also had stale Group values. settings.md's prose and view-routes table still referenced the pre-move /settings paths instead of /admin/settings. Found by an out-of-scope code-review pass while confirming 5a804e2/d772ed2 were clean; these inaccuracies predate both commits (already present in 88f880d) so they're committed separately rather than folded into the admin-admission fix. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
…enance test_method_not_covered_by_the_rule_still_gates claimed to verify that a method not covered by a public-route rule still gates, but it requested an unrelated path with no rule at all rather than a disallowed method on the same path the rule covers — the actually security-relevant case (a GET-only public rule must not let POST bypass maintenance) was untested, and the file's _get helper only issued GET requests so nothing could catch a regression there. Renamed the existing test to test_uncovered_path_still_gates (accurate to what it checks) and added test_wrong_method_on_a_covered_path_still_gates, which POSTs to a GET-only registered path and asserts 503. The underlying PublicRoute.matches/PublicRouteRegistry.matches were already correctly method-aware (confirmed by reading public_routes.py) — this closes a test-coverage gap found during round-1 pass-2 review, not a runtime bug. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
The e2e suite still drove /users/admin, /audit_log, /settings/modules, /feature_flags/ and /branding/. Two specs failed in CI: the command palette waited on **/audit_log** forever, and the user-search spec loaded /users/admin?q=_ and found nothing to assert against. This was missed because e2e is excluded from `make test-py` (-m 'not e2e and not perf'), so every local run stayed green while the E2E smoke job was red — the local gate and CI disagreed about what "tests pass" means. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
Surfaced by running the e2e suite locally after rebasing onto main. SidebarLayout passed only its own menuKey's items to CommandPalette, so once the admin screens moved to adminSidebar, ⌘K from the app shell could no longer reach Users, Settings, Audit Log or any other admin page — and from AdminLayout it could no longer reach the app ones. The palette's own docstring promises "⌘K over everything the sidebar can reach"; that quietly stopped being true. It now indexes both sidebars, deduped by url. Both are already filtered by roles and permissions server-side, so this widens reach without offering anything the viewer cannot open. Also points main's new test_module_settings_render at /admin/settings/; it was written against /settings/ while this branch was moving it. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
antosubashforce-pushed
the
feat/admin-section
branch
from
August 21, 2026 13:50
88f880d to
f18ada8Compare…to end Round 1 pass 3 (post-rebase confirmation) verified by source-reading that resolving the middleware conflict as InertiaCache -> Maintenance -> CommitBeforeResponse is correct: Maintenance's short-circuited 503 is sent through InertiaCache's send-wrapper (its `self.app` is Maintenance), so an Inertia request during maintenance still gets `private, no-store` / dropped ETag / `Vary: X-Inertia` — the exact guarantee GH #272 added InertiaCache for. test_middleware_order.py already pins the *order*; it can't prove a short-circuiting middleware's response actually reaches the wrapper outside it. Add that as a direct test: build the two middlewares in real pipeline order and assert the 503's headers, for both an X-Inertia request (gets the cache guard) and a plain API request (does not, matching the existing JSON-caller behavior). Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
The perf and loadtest suites still drove /users/admin and /audit_log/. Nothing failed — routes_legacy.py 301s them and page.goto follows — so the suites stayed green while quietly measuring an extra redirect hop on every navigation sample, and the loadtest reported timings under a route name that no longer exists. A perf guard that passes while measuring the wrong thing is worse than one that fails, since nothing prompts anyone to look. Neither suite runs in the default gate (perf is excluded by the pytest marker filter, loadtest is a separate Locust script), which is why the URL move missed them. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
Automated high-effort review against refs/remotes/origin/main (--fix) found 10 real issues in the admin-section move and the deep-link/error work that shipped alongside it. All but one fixed here; the remaining one is a pre-existing, unrelated bug left as a judgment call (see below). - No menu item anywhere pointed at /admin: admins landing on /dashboard/ had no click path into the admin section once every admin screen moved out of the main sidebar. Add AdminSectionLink, rendered at the bottom of the plain-sidebar shell only when the viewer has at least one admin entry — mirrors host/routes.py's own admission rule, so it never offers a link that would 403. New "ui.nav.admin" key in both locales. - Dashboard's "Users" tile opened the viewer's own profile instead of user management: menuTarget()'s prefix fallback assumed the Users menu entry still lived under /users, but it moved to /admin/users (admin_view_prefix). Ship the module's admin mount point in the system_info payload (stats.py) and search it first (Home.tsx), matching how the module itself is structured. - Two in-app links (RolesCard.tsx, RolesTab.tsx) still pointed at the pre-move /permissions/... prefix, working today only via the routes_legacy.py 301 shim that is scheduled for removal. - audit_log and feature_flags Browse.tsx targeted the bare /admin/audit-log and /admin/feature-flags prefixes with router.visit(); neither is eligible for _clone_bare_prefix_route's bare-alias (both are contributed via include_router), so every filter/sort change cost an extra 307 round trip. Use the canonical trailing-slash form, matching each module's own MENU_URL. - Google OAuth login never consumed the SESSION_NEXT_KEY deep link AuthMiddleware stashes before bouncing an anonymous visitor to login — the password and Keycloak paths already did. Wire it in the same way: pop, re-sanitise with safe_next_or_none (the value lands in a Location header), fall back to login_redirect_url. Added a source-level test pinning all three completion paths to the same contract, since two of the three need a live identity provider to exercise end to end. - redirects.py's safe_referer_or_root carried its own, weaker inline same-site check alongside the new redirect_safety.safe_next, and accepted at least one shape (backslash-prefixed relative paths) that safe_next correctly rejects. Delegate to safe_next instead of restating its rules, so the two cannot drift apart again. - _INERTIA_ERROR_STATUSES widened to cover 401/429/503 moved exactly the statuses whose headers carry meaning (WWW-Authenticate, Retry-After) onto the page-rendering branch of the error handler, which dropped them — narrowing a contract the handler's own comment still claimed to honor. render_error_page now takes the exception's headers through to both the rendered page and its JSON fallback; maintenance.py's 503 goes through the same path instead of mutating the response after the fact. Frontend's SIGN_IN_STATUSES literal removed in favor of trusting the server's login_url (null unless it should show), so the two lists cannot drift. - host/locales/es.json and packages/ui/locales/es.json were missing every key this branch's error/offline/admin-nav work added to their English counterparts (20+ keys) — Spanish visitors would have seen raw keys or a silent English fallback. Filled in and re-sorted to match en.json's key order; both packages now have full key parity. - tests/test_audit_log.py and tests/test_principal_resolver_integration.py (both outside pytest's `testpaths`, so never collected by `make test-py`) still asserted against the pre-move /audit_log/ and /users/admin view URLs. Retargeted to /admin/audit-log/ and /admin/users/. Left unfixed, out of scope for this pass: retargeting tests/test_principal_resolver_integration.py surfaced a second, wholly unrelated bug in modules/users/users/provider.py (untouched by this branch, predates it entirely) — UsersAuthProvider.resolve_user() takes the bearer-token branch whenever an Authorization header is present and never falls back to the session cookie if that resolution fails, so test_session_wins_over_bad_bearer fails (a valid session + a garbage Bearer header currently 401s instead of authenticating via session). Whether an explicit-but-invalid bearer token should fall back to the session or hard-fail is a security/product judgment call, not a rebase or admin-section correctness bug, so it's flagged rather than changed here. Verified: uv run pytest -q -> 2137 passed, 2 skipped; npm test -> 105 passed; ruff / ty / biome ci / tsc (host + packages/ui) clean; make doctor -> 0 errors (1 pre-existing, unrelated SM003 warning); file-size and hardcoded-string checks clean. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
Found by browser QA. The module tiles resolve a target by searching menu entries under each of the module's prefixes. Preferring the admin prefix outright sent the Dashboard tile to Doctor: dashboard owns both /dashboard and /admin/doctor, and its own screen is the one the tile is for. The rule that fixed the Users tile broke this one. Exact menu matches on either mount point now beat under-prefix guessing on both, which resolves each case on evidence rather than on an ordering that can only ever suit one of them: dashboard hits /dashboard/ exactly, while users has no exact /users entry and so lands on /admin/users/ rather than falling through to the profile page. Also names the users-table row action. It was icon-only with no accessible name — pre-existing (identical to main apart from the href), but this branch already edits that line and a screen-reader user tabbing the table otherwise hears an indistinguishable "Edit" per row with no way to tell which account it opens. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
The tile-target fix hand-rolled trailing-slash normalisation and exact
matching that already exist as trimmed/samePath/isUnder in
packages/ui/src/lib/current-path.ts, which AppTopbar uses for the same
job. Two implementations of "do these paths refer to the same screen"
is how the sidebar highlight and the tiles end up disagreeing.
Behaviourally equivalent — same normalisation, same param order,
trimmed('') === '' preserved.
Found by the round-2 confirming review, which also traced menuTarget
against every installed module's prefixes and menu entries: dashboard
resolves to /dashboard/, users to /admin/users/, and permissions,
site_lock and auth stay inert because they ship no menu entry.
Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kwantosubash
marked this pull request as ready for review
August 21, 2026 15:26
Found by QA, which reported every tab reading as the bare app name. Inertia's head manager only replaces elements carrying the `inertia` attribute. The root template shipped a plain <title>, so the manager left it alone and *appended* its own <title inertia=""> beside it. Two title elements, and the browser uses the first in document order — so the static brand name always won and no <Head title> anywhere in the app had any effect. Marking the template's title head-managed makes the manager replace it, while the server still renders the branded name for the pre-hydration tab. App-wide, not specific to the admin section: /dashboard/ predates this branch, sets a static <Head title="Dashboard">, and was equally bare. Covered by e2e assertions on both the text and the element count — the count is the actual defect, since the text only goes wrong because of the ordering a second title produces. Verified by reverting the attribute: all five fail, and pass again with it. The branding test asserted the exact <title> markup; relaxed to the text so an attribute it is not about cannot fail it. Also adds unit coverage for OfflineBanner, whose "back online" confirmation QA reported missing. It was not missing: the sequence works in a real browser, and the reading came from monkeypatching navigator.onLine and sampling ~150ms later. Five fake-timer tests now pin the offline, restored, cleared and re-armed states deterministically rather than leaving it to a browser clock. Claude-Session: https://claude.ai/code/session_01HCFXBubRRWDDBeyqYRZ1kw
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
/admin/*, intoMenuSection.ADMIN_SIDEBARandAdminLayout, grouped Access / Appearance / System, with a/adminoverview assembled from the viewer's own filtered sidebar. Old URLs 301. Only view URLs moved —/api/*is untouched.AuthMiddlewarestashed it but the local provider never read it back, so every login landed on the default instead of the requested page. Also closes an open redirect:Login.tsxread?next=from the query string.One framework addition:
ModuleMeta.admin_view_prefix+ModuleBase.register_admin_routes, both additive and defaulting to no-op. A module gets one view router, whichuserscan't express — it serves/users/loginand its management CRUD from one package.Verification
6f8b791Full report:https://claude.ai/code/artifact/9a281369-d3a6-42f9-a688-1d6946124af4
QA
Full browser cycle — 1 iteration, 1 bug found and fixed, 0 remaining. Three serialized browser agents plus an HTTP battery covering the auth bounce on all 8 admin routes, the deep link with its query string, all 7 legacy 301s, error statuses, and confirmation
/api/*didn't move.Ten failing browser checks resolved to five distinct issues. Four were proven not to belong to this branch, each with a control experiment or a diff against
main— notably per-page document titles, where the control (/dashboard/, which predates this work and sets a static title) is also bare, so the breakage is app-wide.Notable fixes found by review
/admin/doctor/had no authorization dependency at all — any signed-in account could read migration status, module list and system info.render_error_pagediscarded the exception's headers — widening the rendered status set to 401/429/503 therefore strippedWWW-AuthenticateandRetry-Afterfrom exactly the statuses that need them.Known, deferred
None of these is a defect this branch introduced; they are recorded rather than dropped.
tests/is not intestpaths, so two files there are collected by neithermake test-pynor CI — and one fails today: the auth provider takes the bearer branch unconditionally and never falls back to the session cookie. Identical tomain. Adding the path would turn CI red on a security decision (should an invalid bearer fall back, or hard-fail?) that deserves its own change./admin, the doctor route, maintenance middleware). Consolidating a FastAPI dependency and raw ASGI middleware is new framework surface, not a fix.Test plan
/adminas an admin and confirm the grouped sidebar and matching tool cards/admin/settings/?tab=modulessigned out — confirm you land back there after login, query string intact/users/adminstill resolves