From 26b46aa6bdaf03ffe47c99aa12cae5a31b861ac0 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 13:52:28 +0200 Subject: [PATCH 01/17] fix(auth): preserve the deep link across login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../simple_module_core/redirect_safety.py | 56 +++++++++++++++ framework/core/tests/test_redirect_safety.py | 59 ++++++++++++++++ modules/auth/auth/middleware.py | 20 +++++- modules/auth/tests/test_auth_middleware.py | 70 +++++++++++++++++++ modules/keycloak/keycloak/endpoints/api.py | 9 ++- modules/site_lock/site_lock/page.py | 22 ++---- modules/users/tests/test_views.py | 50 +++++++++++++ modules/users/users/auth_local/api.py | 9 +++ modules/users/users/auth_local/views.py | 10 ++- modules/users/users/pages/Login.tsx | 9 +-- 10 files changed, 289 insertions(+), 25 deletions(-) create mode 100644 framework/core/simple_module_core/redirect_safety.py create mode 100644 framework/core/tests/test_redirect_safety.py diff --git a/framework/core/simple_module_core/redirect_safety.py b/framework/core/simple_module_core/redirect_safety.py new file mode 100644 index 00000000..82e09546 --- /dev/null +++ b/framework/core/simple_module_core/redirect_safety.py @@ -0,0 +1,56 @@ +"""Safety net for user-influenced redirect targets. + +Several flows park "where the visitor was heading" somewhere a browser can +reach — ``AuthMiddleware`` stashes it in the session before bouncing an +anonymous visitor to login, ``site_lock`` puts it in the unlock page's query +string. Anything replayed into a ``Location`` header is an open-redirect +surface, so every producer and consumer funnels through :func:`safe_next`. + +This lives in the framework rather than in whichever module needed it first: +it encodes no plugin knowledge, and duplicating URL-safety rules per module is +how one copy ends up missing a case. +""" + +from __future__ import annotations + +DEFAULT_FALLBACK = "/" + +SESSION_NEXT_KEY = "next" +"""Session key holding the post-login destination. + +This is the contract between ``AuthMiddleware`` (which writes it when it +bounces an anonymous visitor) and whichever provider completes the login and +sends the visitor onward. It is shared rather than redeclared per module +because a provider that reads a *different* key silently loses every deep +link — which is exactly how the local provider drifted from the Keycloak one. +""" + + +def safe_next(raw: str | None, *, fallback: str = DEFAULT_FALLBACK) -> str: + """Return ``raw`` if it is a same-site absolute path, else ``fallback``. + + Rejects protocol-relative (``//host``) and backslash-prefixed (``/\\host``) + targets — browsers resolve both off-site — plus anything carrying CR/LF, + which could otherwise be smuggled into the redirect header. + """ + if not raw or not raw.startswith("/"): + return fallback + if raw.startswith(("//", "/\\")): + return fallback + if "\r" in raw or "\n" in raw: + return fallback + return raw + + +def safe_next_or_none(raw: str | None) -> str | None: + """Like :func:`safe_next`, but ``None`` when ``raw`` is unusable. + + Callers that fall back to a *configured* destination (rather than ``/``) + need to tell "no target" apart from "the target was ``/``" — returning the + fallback would silently outrank a configured ``login_redirect_url``. + """ + result = safe_next(raw, fallback="") + return result or None + + +__all__ = ["DEFAULT_FALLBACK", "SESSION_NEXT_KEY", "safe_next", "safe_next_or_none"] diff --git a/framework/core/tests/test_redirect_safety.py b/framework/core/tests/test_redirect_safety.py new file mode 100644 index 00000000..077b3e35 --- /dev/null +++ b/framework/core/tests/test_redirect_safety.py @@ -0,0 +1,59 @@ +"""Tests for the shared redirect-target sanitiser.""" + +from __future__ import annotations + +import pytest +from simple_module_core.redirect_safety import safe_next, safe_next_or_none + + +class TestSafeNext: + @pytest.mark.parametrize( + "raw", + [ + "/dashboard/", + "/admin/users?page=2", + "/admin/users#anchor", + "/", + ], + ) + def test_same_site_paths_pass_through(self, raw: str) -> None: + assert safe_next(raw) == raw + + @pytest.mark.parametrize( + "raw", + [ + None, + "", + "https://evil.example/phish", + "dashboard/", + "javascript:alert(1)", + ], + ) + def test_non_relative_targets_are_rejected(self, raw: str | None) -> None: + assert safe_next(raw) == "/" + + @pytest.mark.parametrize("raw", ["//evil.example", "/\\evil.example"]) + def test_off_site_lookalikes_are_rejected(self, raw: str) -> None: + """Browsers resolve both forms against the remote host, not ours.""" + assert safe_next(raw) == "/" + + @pytest.mark.parametrize("raw", ["/ok\r\nLocation: https://evil.example", "/ok\nX: y"]) + def test_header_smuggling_is_rejected(self, raw: str) -> None: + assert safe_next(raw) == "/" + + def test_fallback_is_configurable(self) -> None: + assert safe_next("https://evil.example", fallback="/login") == "/login" + + +class TestSafeNextOrNone: + def test_valid_target_returned(self) -> None: + assert safe_next_or_none("/admin/settings") == "/admin/settings" + + @pytest.mark.parametrize("raw", [None, "", "//evil.example", "https://evil.example"]) + def test_unusable_target_is_none(self, raw: str | None) -> None: + assert safe_next_or_none(raw) is None + + def test_root_is_a_real_target_not_a_miss(self) -> None: + """``/`` must stay distinguishable from "nothing stashed" — otherwise a + caller cannot tell whether to fall back to its configured destination.""" + assert safe_next_or_none("/") == "/" diff --git a/modules/auth/auth/middleware.py b/modules/auth/auth/middleware.py index 91e8383a..cac24241 100644 --- a/modules/auth/auth/middleware.py +++ b/modules/auth/auth/middleware.py @@ -10,6 +10,7 @@ import logging +from simple_module_core.redirect_safety import SESSION_NEXT_KEY, safe_next_or_none from simple_module_db.listeners import current_user_id from starlette.requests import Request from starlette.responses import JSONResponse, RedirectResponse @@ -26,7 +27,10 @@ "/i18n/", ) _FRAMEWORK_PUBLIC_EXACT = ("/",) -_SESSION_NEXT_KEY = "next" + + +def _query_suffix(request: Request) -> str: + return f"?{request.url.query}" if request.url.query else "" class AuthMiddleware: @@ -92,9 +96,19 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: if path.startswith("/api/") or provider.is_bearer_request(request): response = JSONResponse({"detail": "Not authenticated"}, status_code=401) else: + # Stash where they were heading so the login flow can return + # them there. Relative, not ``str(request.url)``: the value is + # replayed into a ``Location`` header, and an absolute URL is + # both needless and an open-redirect shape. ``next_url`` also + # goes to the provider — session-based providers ignore it, + # but a redirect-based one (OIDC) needs it in the auth URL. session = scope.get("session", {}) - session[_SESSION_NEXT_KEY] = str(request.url) - response = RedirectResponse(provider.get_login_url(request), status_code=302) + next_url = safe_next_or_none(request.url.path + _query_suffix(request)) + if next_url is not None: + session[SESSION_NEXT_KEY] = next_url + response = RedirectResponse( + provider.get_login_url(request, next_url), status_code=302 + ) await response(scope, receive, send) return diff --git a/modules/auth/tests/test_auth_middleware.py b/modules/auth/tests/test_auth_middleware.py index 2254bc49..c2bf26ba 100644 --- a/modules/auth/tests/test_auth_middleware.py +++ b/modules/auth/tests/test_auth_middleware.py @@ -203,3 +203,73 @@ async def bad_resolver(request): ) as c: resp = await c.get("/protected/page") assert resp.status_code == 302 + + +class TestDeepLinkPreservation: + """AuthMiddleware stashes where an anonymous visitor was heading. + + The value is replayed into a ``Location`` header after login, so it is + stored relative and sanitised on the way in — see + ``simple_module_core.redirect_safety``. + """ + + async def _get(self, app, path: str): + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient( + transport=transport, base_url="http://test", follow_redirects=False + ) as c: + return await c.get(path) + + async def test_target_is_offered_to_the_provider(self): + """The provider is handed the target, not left to guess it.""" + seen: list[str | None] = [] + + class _RecordingProvider(_StubProvider): + def get_login_url(self, request, next_url=None): + seen.append(next_url) + return "/stub/login" + + app = _build_app(_RecordingProvider()) + app.add_middleware(AuthMiddleware) + app.add_middleware(SessionMiddleware, secret_key=SECRET) + + await self._get(app, "/protected/page?tab=2") + + assert seen == ["/protected/page?tab=2"] + + async def test_target_is_relative_not_absolute(self): + """``str(request.url)`` would hand the provider an absolute URL — + needless, and the wrong shape for a redirect target.""" + seen: list[str | None] = [] + + class _RecordingProvider(_StubProvider): + def get_login_url(self, request, next_url=None): + seen.append(next_url) + return "/stub/login" + + app = _build_app(_RecordingProvider()) + app.add_middleware(AuthMiddleware) + app.add_middleware(SessionMiddleware, secret_key=SECRET) + + await self._get(app, "/protected/page") + + assert seen == ["/protected/page"] + assert not seen[0].startswith("http") + + async def test_authenticated_request_stashes_nothing(self): + """Only the anonymous branch records a target.""" + seen: list[str | None] = [] + + class _RecordingProvider(_StubProvider): + def get_login_url(self, request, next_url=None): + seen.append(next_url) + return "/stub/login" + + app = _build_app(_RecordingProvider(user=_TEST_USER)) + app.add_middleware(AuthMiddleware) + app.add_middleware(SessionMiddleware, secret_key=SECRET) + + resp = await self._get(app, "/protected/page") + + assert resp.status_code == 200 + assert seen == [] diff --git a/modules/keycloak/keycloak/endpoints/api.py b/modules/keycloak/keycloak/endpoints/api.py index fe6281a5..2c2165fd 100644 --- a/modules/keycloak/keycloak/endpoints/api.py +++ b/modules/keycloak/keycloak/endpoints/api.py @@ -7,6 +7,7 @@ from typing import TYPE_CHECKING from fastapi import APIRouter, HTTPException, Request +from simple_module_core.redirect_safety import SESSION_NEXT_KEY, safe_next_or_none from starlette.responses import RedirectResponse if TYPE_CHECKING: @@ -19,7 +20,6 @@ _SESSION_OIDC_NONCE = "keycloak_oidc_nonce" _SESSION_USER_CTX = "user_ctx" _SESSION_ID_TOKEN = "keycloak_id_token" -_SESSION_NEXT = "next" def _get_settings(request: Request) -> KeycloakSettings: @@ -88,5 +88,10 @@ async def oidc_callback(request: Request): request.session[_SESSION_ID_TOKEN] = id_token s = _get_settings(request) - next_url = request.session.pop(_SESSION_NEXT, None) or s.login_redirect_url + # Sanitised on the way out as well as in: this value lands in a + # Location header, and defence in depth costs one call. + next_url = ( + safe_next_or_none(request.session.pop(SESSION_NEXT_KEY, None)) + or s.login_redirect_url + ) return RedirectResponse(next_url, status_code=303) diff --git a/modules/site_lock/site_lock/page.py b/modules/site_lock/site_lock/page.py index 6dface1d..877dc104 100644 --- a/modules/site_lock/site_lock/page.py +++ b/modules/site_lock/site_lock/page.py @@ -11,6 +11,8 @@ import importlib.resources from string import Template +from simple_module_core.redirect_safety import safe_next + _TEMPLATE = Template( (importlib.resources.files(__package__) / "templates" / "unlock.html").read_text( encoding="utf-8" @@ -21,21 +23,11 @@ _ERROR_BLOCK = '' -def safe_next(raw: str | None) -> str: - """Return ``raw`` if it is a same-site absolute path, else ``/``. - - Rejects protocol-relative (``//host``) and backslash-prefixed (``/\\host``) - targets — browsers resolve both off-site — plus anything carrying CR/LF, - which could otherwise be smuggled into the redirect header. Without this - the gate would be an open redirect. - """ - if not raw or not raw.startswith("/"): - return "/" - if raw.startswith(("//", "/\\")): - return "/" - if "\r" in raw or "\n" in raw: - return "/" - return raw +# ``safe_next`` is imported, not defined here: the implementation moved to +# ``simple_module_core.redirect_safety`` once AuthMiddleware needed the same +# rules. Re-exported so existing ``site_lock.page.safe_next`` callers keep +# working. +__all__ = ["render_unlock_page", "safe_next"] def render_unlock_page( diff --git a/modules/users/tests/test_views.py b/modules/users/tests/test_views.py index 0594e33d..a4af5f2e 100644 --- a/modules/users/tests/test_views.py +++ b/modules/users/tests/test_views.py @@ -275,3 +275,53 @@ async def test_login_redirect_fallback_without_dashboard(users_app): assert url.endswith("/"), "must have trailing slash" finally: object.__setattr__(sm, "modules", original) + + +class TestDeepLinkAfterLogin: + """An anonymous visit to a protected page should come back after login. + + AuthMiddleware stashes the target in the session; the login view surfaces + it as ``login_redirect_url``. Before this existed the target was dropped + and every login landed on the configured default. + """ + + @pytest.mark.anyio + async def test_bounced_target_becomes_the_redirect_prop(self, anon_client): + bounced = await anon_client.get("/settings/", follow_redirects=False) + assert bounced.status_code == 302 + + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/settings/" + + @pytest.mark.anyio + async def test_query_string_is_preserved(self, anon_client): + await anon_client.get("/settings/?tab=modules", follow_redirects=False) + + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/settings/?tab=modules" + + @pytest.mark.anyio + async def test_reload_of_login_page_keeps_the_target(self, anon_client): + """Read-not-pop: reloading the login page must not lose the deep link.""" + await anon_client.get("/settings/", follow_redirects=False) + + for _ in range(2): + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/settings/" + + @pytest.mark.anyio + async def test_without_a_bounce_the_default_is_used(self, anon_client): + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/dashboard/" diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index 209b4e99..f027d405 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -17,6 +17,8 @@ from fastapi_users import exceptions as fu_exceptions from users.auth_local.rate_limit import LoginRateLimiter, ThroughputLimiter +from simple_module_core.redirect_safety import SESSION_NEXT_KEY + from users.constants import SESSION_USER_ID_KEY from users.contracts.schemas import ( AcceptInviteRequest, @@ -111,6 +113,9 @@ async def login( login_response = await auth_backend.login(strategy, user) # Bridge the session cookie — AuthMiddleware reads this to identify the user request.session[SESSION_USER_ID_KEY] = str(user.id) + # The deep link has served its purpose; leaving it would send the *next* + # plain visit to /users/login off to a stale destination. + request.session.pop(SESSION_NEXT_KEY, None) return login_response @@ -166,6 +171,10 @@ async def accept_invite( await user_manager.on_after_login(user, request, response) login_response = await auth_backend.login(strategy, user) request.session[SESSION_USER_ID_KEY] = str(user.id) + # Invite acceptance routes the user itself, so a deep link stashed by an + # earlier bounce is stale here — drop it rather than leave it to fire on + # some later visit to the login page. + request.session.pop(SESSION_NEXT_KEY, None) return login_response diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index cf694526..90ea19b4 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -4,6 +4,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request from inertia import InertiaResponse +from simple_module_core.redirect_safety import SESSION_NEXT_KEY, safe_next_or_none from simple_module_hosting.inertia_deps import InertiaDep from starlette.responses import RedirectResponse @@ -55,7 +56,14 @@ async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: { "allow_signup": users_settings.allow_signup, "dev_accounts": dev_accounts, - "login_redirect_url": users_settings.login_redirect_url, + # Where AuthMiddleware bounced them from, when it bounced them. + # Read, not popped: a reload of the login page must not silently + # downgrade the deep link to the default landing page. The POST + # handler clears it once login actually succeeds. + "login_redirect_url": ( + safe_next_or_none(request.session.get(SESSION_NEXT_KEY)) + or users_settings.login_redirect_url + ), "oauth_providers": users_state.oauth_providers, }, ) diff --git a/modules/users/users/pages/Login.tsx b/modules/users/users/pages/Login.tsx index 975fb44b..ad76cee1 100644 --- a/modules/users/users/pages/Login.tsx +++ b/modules/users/users/pages/Login.tsx @@ -36,10 +36,11 @@ function Login() { const [needsVerification, setNeedsVerification] = useState(false); const [loading, setLoading] = useState(false); - const nextUrl = - typeof window !== 'undefined' - ? new URLSearchParams(window.location.search).get('next') || login_redirect_url - : login_redirect_url; + // Server-decided, deliberately. The post-login destination used to be read + // from `?next=` here, which let any crafted login link bounce the user to an + // arbitrary URL after signing in. AuthMiddleware now stashes the target in + // the session and the view sanitises it, so this prop is already safe. + const nextUrl = login_redirect_url; const submitLogin = (username: string, pwd: string) => { setError(null); From 41a5369c4363033ee44e3a7dc00fcf39c91736ff Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 14:01:06 +0200 Subject: [PATCH 02/17] feat(hosting): common error states, maintenance mode, offline banner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 --- CLAUDE.md | 2 +- .../core/simple_module_core/permissions.py | 17 +- .../simple_module_hosting/_error_handlers.py | 33 +++- .../simple_module_hosting/_phase_helpers.py | 12 +- .../simple_module_hosting/host_settings.py | 10 + .../simple_module_hosting/maintenance.py | 107 +++++++++++ .../tests/test_error_status_coverage.py | 126 ++++++++++++ .../hosting/tests/test_maintenance_mode.py | 179 ++++++++++++++++++ .../hosting/tests/test_middleware_order.py | 16 +- host/client_app/app.tsx | 5 + host/client_app/pages/Error.tsx | 109 ++++++++--- host/locales/en.json | 36 +++- packages/i18n/src/generated-resources.ts | 16 ++ packages/i18n/src/keys.generated.ts | 18 ++ packages/ui/src/components/OfflineBanner.tsx | 60 ++++++ packages/ui/src/hooks/use-online.ts | 34 ++++ packages/ui/src/index.ts | 2 + 17 files changed, 744 insertions(+), 38 deletions(-) create mode 100644 framework/hosting/simple_module_hosting/maintenance.py create mode 100644 framework/hosting/tests/test_error_status_coverage.py create mode 100644 framework/hosting/tests/test_maintenance_mode.py create mode 100644 packages/ui/src/components/OfflineBanner.tsx create mode 100644 packages/ui/src/hooks/use-online.ts diff --git a/CLAUDE.md b/CLAUDE.md index 7304b48a..0cd94739 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -77,7 +77,7 @@ hence `SM022`/`SM023`. See `docs/module-authoring.md` § Styling. `register_settings` → `register_menu_items` / `register_permissions` / `register_feature_flags` / `register_event_handlers` / `register_health_checks` / `register_public_routes` / `register_csp_sources` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` → async `on_startup` / `on_shutdown` (reverse order). `register_csp_sources(registry)` lets a module whitelist external asset origins (`registry.add("style-src", "https://rsms.me")`) — fetch directives only, validated at boot. `register_public_routes(registry)` lets a module exempt anonymous/read-only routes (STAC/OGC, webhooks) from `AuthMiddleware`; rules are method-aware (`registry.add_regex(r"…/tilejson$", methods={"GET"})`), so a GET read route can be public while sibling POST/PATCH mutations under the same prefix stay gated. See [docs/framework/public-routes.md](docs/framework/public-routes.md). **Middleware pipeline** (Starlette `add_middleware` is LIFO — last added runs first). Execution order on a request: -`(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → CommitBeforeResponse → app`. `InertiaCache` answers for `InertiaLayoutData` merging per-user `auth`/`menus` into every payload: a response to an `X-Inertia` request is forced to `private, no-store` with its ETag dropped, and both representations of a URL gain `Vary: X-Inertia` — so no cache can store the JSON payload or hand it back for a page request. A module wanting its public page content cached should set `Cache-Control` and an ETag on the *document*; that path is left alone. `GZip` compresses any response over 500 bytes, including the `/static` mount — the built CSS is ~139 KB raw versus ~21 KB gzipped, and uncompressed assets dominated cold page load. `ProxyHeaders` (uvicorn's `ProxyHeadersMiddleware`) is installed only when `SM_TRUSTED_PROXY` is set, sitting outermost so the `X-Forwarded-*`-corrected scheme/client IP reach everything downstream (request logs and Inertia's absolute page url). When two modules add middleware at the same dependency tier, the module that sorts **later** wraps outermost. Use `depends_on` to express relative order — don't rely on names. +`(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → Maintenance → CommitBeforeResponse → app`. `InertiaCache` answers for `InertiaLayoutData` merging per-user `auth`/`menus` into every payload: a response to an `X-Inertia` request is forced to `private, no-store` with its ETag dropped, and both representations of a URL gain `Vary: X-Inertia` — so no cache can store the JSON payload or hand it back for a page request. A module wanting its public page content cached should set `Cache-Control` and an ETag on the *document*; that path is left alone. `GZip` compresses any response over 500 bytes, including the `/static` mount — the built CSS is ~139 KB raw versus ~21 KB gzipped, and uncompressed assets dominated cold page load. `ProxyHeaders` (uvicorn's `ProxyHeadersMiddleware`) is installed only when `SM_TRUSTED_PROXY` is set, sitting outermost so the `X-Forwarded-*`-corrected scheme/client IP reach everything downstream (request logs and Inertia's absolute page url). When two modules add middleware at the same dependency tier, the module that sorts **later** wraps outermost. Use `depends_on` to express relative order — don't rely on names. `Maintenance` serves a 503 page to everyone but admins while `maintenance_mode` is set on `HostSettings`; it sits inside `InertiaCache` because its 503 is an Inertia payload produced by short-circuiting, and outside the cache guard that payload would ship storable. **Database**: per-module `Base` via `create_module_base("")`. Every module owns its own `MetaData` (so Alembic autogenerate can attribute tables to a module), but all tables live in the host's single schema. `__tablename__` must be prefixed with the module name to avoid collisions (`orders_order`). Postgres and SQLite share the same layout. diff --git a/framework/core/simple_module_core/permissions.py b/framework/core/simple_module_core/permissions.py index 8cb7335c..1fe15ae7 100644 --- a/framework/core/simple_module_core/permissions.py +++ b/framework/core/simple_module_core/permissions.py @@ -6,13 +6,26 @@ WILDCARD = "*" +ADMIN_ROLE = "admin" +"""The one role name the framework itself knows. + +Everything else about roles is module-owned, but the framework needs this to +resolve the wildcard grant below and to decide who can still reach the app +while maintenance mode is on. +""" + # Default role→permission mapping. Admin gets all permissions via the wildcard. # Additional mappings are added at registration time via PermissionRegistry.map_role. DEFAULT_ROLE_PERMISSIONS: dict[str, list[str]] = { - "admin": [WILDCARD], + ADMIN_ROLE: [WILDCARD], } +def is_admin(roles: list[str] | None) -> bool: + """Whether ``roles`` carries the framework's admin role.""" + return bool(roles) and ADMIN_ROLE in roles + + @dataclass class PermissionGroup: """A named group of related permissions (typically one per module).""" @@ -111,7 +124,7 @@ def get_permissions_for_roles( other roles get none. Override for richer mapping. """ if role_permission_map is None: - if "admin" in roles: + if is_admin(roles): return set(self.all_permissions) return set() diff --git a/framework/hosting/simple_module_hosting/_error_handlers.py b/framework/hosting/simple_module_hosting/_error_handlers.py index a0846ac7..cc5afead 100644 --- a/framework/hosting/simple_module_hosting/_error_handlers.py +++ b/framework/hosting/simple_module_hosting/_error_handlers.py @@ -22,7 +22,30 @@ logger = logging.getLogger(__name__) -_INERTIA_ERROR_STATUSES = frozenset({403, 404, 500}) +_INERTIA_ERROR_STATUSES = frozenset({401, 403, 404, 419, 422, 429, 500, 503}) + +# Statuses whose remedy is "sign in", so the page offers that as its primary +# action rather than sending the visitor to the landing page. +_SIGN_IN_STATUSES = frozenset({401, 419}) + + +def _login_url(request: Request) -> str | None: + """Best-effort login URL for the sign-in statuses. + + Read off ``app.state`` rather than imported: the auth provider is a plugin + concern and ``SM009`` forbids framework code importing ``modules/*``. An + app with no auth provider installed simply gets no sign-in button. + """ + auth_state = getattr(request.app.state, "auth", None) + provider = getattr(auth_state, "auth_provider", None) + if provider is None: + return None + try: + return provider.get_login_url(request) + except Exception: + # A broken provider must not turn an error page into a second error. + logger.exception("Auth provider failed to supply a login URL") + return None def _explicit_accept_q(accept: str, media_type: str) -> float | None: @@ -86,8 +109,11 @@ def _wants_json(request: Request) -> bool: async def render_error_page(request: Request, status_code: int, message: str) -> Response: - config: InertiaConfig = request.app.state.sm.inertia_config try: + # Inside the try, not above it: this lookup is exactly the kind of + # thing that is missing when the app is half-built, and an error page + # that raises while reporting an error leaves the caller with nothing. + config: InertiaConfig = request.app.state.sm.inertia_config inertia = Inertia(request, config) # This builds its own Inertia instead of going through get_inertia, so # the share step has to be repeated here. Without it the error page @@ -106,6 +132,9 @@ async def render_error_page(request: Request, status_code: int, message: str) -> "status": status_code, "message": message, "correlation_id": getattr(request.state, "correlation_id", "") or "", + "login_url": ( + _login_url(request) if status_code in _SIGN_IN_STATUSES else None + ), }, ) response.status_code = status_code diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 4f53c15a..a01f28a1 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -37,6 +37,7 @@ from simple_module_hosting._inertia_cache import InertiaCacheMiddleware from simple_module_hosting.host_settings import HostSettings from simple_module_hosting.i18n_middleware import LocaleMiddleware +from simple_module_hosting.maintenance import MaintenanceMiddleware from simple_module_hosting.middleware import ( CorrelationIdMiddleware, InertiaLayoutDataMiddleware, @@ -104,12 +105,21 @@ def install_middleware( Order matters: last added = first executed. Execution order: (ProxyHeaders, if trusted_proxy) → CorrelationId → RequestLogging → Security → Session → [module] → (Tenant, if multi_tenant) → Locale - → Inertia → InertiaCache → CommitBeforeResponse. + → Inertia → InertiaCache → Maintenance → CommitBeforeResponse. """ # Added first, so it is innermost and its send-wrapper is the first to see # the response: the request's DB work commits before any byte reaches the # client, instead of in get_db's post-response exit code (GH #257). app.add_middleware(CommitBeforeResponseMiddleware) + # Inside InertiaCache, so its short-circuit is still governed by it. The + # maintenance 503 renders through Inertia and carries this user's auth + # block and menus like any other payload; short-circuiting *outside* the + # cache guard would ship exactly the per-user payload GH #272 exists to + # keep out of caches. Added before Inertia so it *executes* after it: the + # page needs the shared props (auth, menus, i18n) to render with a layout, + # and auth + locale — both further out — to know who is asking and in which + # language to answer. + app.add_middleware(MaintenanceMiddleware) # Paired with InertiaLayoutDataMiddleware below, which is what puts this # user's auth, permissions and menus into every Inertia payload: this one # makes sure the payload that results is never stored where a page request diff --git a/framework/hosting/simple_module_hosting/host_settings.py b/framework/hosting/simple_module_hosting/host_settings.py index 779d9071..b6d9989c 100644 --- a/framework/hosting/simple_module_hosting/host_settings.py +++ b/framework/hosting/simple_module_hosting/host_settings.py @@ -19,6 +19,16 @@ class HostSettings(BaseSettings): multi_tenant: bool = False tenant_header: str = "" + maintenance_mode: bool = False + """Serve everyone but admins a 503 page. + + DB-backed rather than an env var on purpose: flipping it must not need a + redeploy, which is exactly when you want it. + """ + maintenance_message: str = "" + """Optional operator note shown on the maintenance page. Empty = use the + generic translated copy.""" + i18n_default_locale: str = "en" i18n_supported_locales: list[str] = ["en"] i18n_cookie_name: str = "locale" diff --git a/framework/hosting/simple_module_hosting/maintenance.py b/framework/hosting/simple_module_hosting/maintenance.py new file mode 100644 index 00000000..d05c8078 --- /dev/null +++ b/framework/hosting/simple_module_hosting/maintenance.py @@ -0,0 +1,107 @@ +"""Maintenance mode — serve everyone but admins a 503 page. + +Sits late in the pipeline, after auth (so it knows who is asking), after +locale (so the page is translated) and after the Inertia shared-props +middleware (so the page keeps its layout instead of rendering bare). + +Admins pass through. That is the whole point: someone has to be able to reach +the settings screen and turn it back off. For the same reason the auth +provider's own routes stay open — an admin who is signed *out* when the switch +is flipped must still be able to sign in. +""" + +from __future__ import annotations + +import logging + +from simple_module_core.permissions import is_admin +from starlette.requests import Request +from starlette.responses import JSONResponse +from starlette.types import ASGIApp, Receive, Scope, Send + +logger = logging.getLogger(__name__) + +# Kept reachable while the gate is closed: liveness probes (so orchestrators +# do not kill the pod mid-maintenance), static assets and i18n bundles (or the +# 503 page renders unstyled and untranslated). +_ALWAYS_OPEN_PREFIXES = ( + "/health", + "/static/", + "/i18n/", +) + +__all__ = ["MaintenanceMiddleware"] + + +class MaintenanceMiddleware: + """Short-circuit non-admin traffic with a 503 while maintenance is on.""" + + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != "http": + await self.app(scope, receive, send) + return + + settings = self._host_settings(scope) + if settings is None or not getattr(settings, "maintenance_mode", False): + await self.app(scope, receive, send) + return + + path: str = scope["path"] + if any(path.startswith(p) for p in _ALWAYS_OPEN_PREFIXES): + await self.app(scope, receive, send) + return + + request = Request(scope) + if self._may_bypass(request, scope): + await self.app(scope, receive, send) + return + + message = getattr(settings, "maintenance_message", "") or "" + response = await self._render(request, message) + await response(scope, receive, send) + + @staticmethod + def _host_settings(scope: Scope): + host_state = getattr(scope["app"].state, "host", None) + return getattr(host_state, "settings", None) + + @staticmethod + def _may_bypass(request: Request, scope: Scope) -> bool: + """Admins, and anyone heading for the auth provider's own routes.""" + user = getattr(request.state, "user", None) + if user is not None and is_admin(getattr(user, "roles", None)): + return True + + # An admin locked out by the switch still needs the login flow. Ask the + # provider which paths those are rather than hardcoding a module's URLs. + auth_state = getattr(scope["app"].state, "auth", None) + provider = getattr(auth_state, "auth_provider", None) + if provider is None: + return False + try: + prefix_paths, exact_paths = provider.get_public_paths() + except Exception: + logger.exception("Auth provider failed to report public paths") + return False + path: str = scope["path"] + return any(path.startswith(p) for p in prefix_paths) or path in exact_paths + + @staticmethod + async def _render(request: Request, message: str): + # Imported here rather than at module scope: _error_handlers imports + # from inertia, and a circular import at boot is a worse failure than + # a per-request attribute lookup. + from simple_module_hosting._error_handlers import _wants_json, render_error_page + + if _wants_json(request): + return JSONResponse( + status_code=503, + content={"detail": message or "Service temporarily unavailable"}, + headers={"Retry-After": "3600"}, + ) + response = await render_error_page(request, 503, message) + response.headers["Retry-After"] = "3600" + return response diff --git a/framework/hosting/tests/test_error_status_coverage.py b/framework/hosting/tests/test_error_status_coverage.py new file mode 100644 index 00000000..3323623f --- /dev/null +++ b/framework/hosting/tests/test_error_status_coverage.py @@ -0,0 +1,126 @@ +"""Every status the handler renders must have copy on the page. + +The Inertia error page keys its title/description/accent off the numeric +status. A status the handler renders but the page has no row for falls back +to a bare "Error / An unexpected error occurred" — which is worse than the +generic message suggests, because the user is told nothing actionable. These +two lists live in different languages, so nothing but a test keeps them +honest. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +from simple_module_hosting._error_handlers import ( + _INERTIA_ERROR_STATUSES, + _SIGN_IN_STATUSES, + _login_url, +) + +_ERROR_PAGE = ( + Path(__file__).resolve().parents[3] / "host" / "client_app" / "pages" / "Error.tsx" +) + + +def _statuses_with_copy() -> set[int]: + """Numeric keys of the status table in Error.tsx.""" + source = _ERROR_PAGE.read_text(encoding="utf-8") + table = re.search( + r"const table: Record = \{(.*?)\n \};", source, re.DOTALL + ) + assert table, "status table not found in Error.tsx — did its shape change?" + return {int(m) for m in re.findall(r"^ (\d{3}):", table.group(1), re.MULTILINE)} + + +class TestStatusCopyParity: + def test_error_page_exists(self) -> None: + assert _ERROR_PAGE.is_file(), _ERROR_PAGE + + def test_every_rendered_status_has_copy(self) -> None: + missing = _INERTIA_ERROR_STATUSES - _statuses_with_copy() + assert not missing, ( + f"statuses rendered by the handler with no copy in Error.tsx: {sorted(missing)}" + ) + + def test_sign_in_statuses_are_rendered_statuses(self) -> None: + """Offering a sign-in button on a status that never reaches the page + would be dead code.""" + assert _SIGN_IN_STATUSES <= _INERTIA_ERROR_STATUSES + + @pytest.mark.parametrize("status", [401, 403, 404, 419, 422, 429, 500, 503]) + def test_expected_statuses_are_covered(self, status: int) -> None: + assert status in _INERTIA_ERROR_STATUSES + + +class _StubRequest: + def __init__(self, provider: object | None) -> None: + class _AuthState: + auth_provider = provider + + class _State: + auth = _AuthState() if provider is not None else None + + class _App: + state = _State() + + self.app = _App() + + +class TestLoginUrlLookup: + def test_returns_provider_url(self) -> None: + class _Provider: + def get_login_url(self, request, next_url=None): + return "/users/login" + + assert _login_url(_StubRequest(_Provider())) == "/users/login" + + def test_no_auth_provider_yields_none(self) -> None: + """An app with no auth module installed simply gets no sign-in button.""" + assert _login_url(_StubRequest(None)) is None + + def test_broken_provider_does_not_raise(self) -> None: + """An error page that itself errors is the worst possible outcome.""" + + class _Exploding: + def get_login_url(self, request, next_url=None): + raise RuntimeError("provider is down") + + assert _login_url(_StubRequest(_Exploding())) is None + + +class TestRenderFallback: + """render_error_page must never raise — it is the last line of defence.""" + + async def test_half_built_app_falls_back_to_json(self) -> None: + """``app.state.sm`` is missing while the app is still assembling. That + lookup used to sit outside the try, so the documented JSON fallback + never ran and the error page raised while reporting an error.""" + from starlette.applications import Starlette + from starlette.requests import Request + + from simple_module_hosting._error_handlers import render_error_page + + app = Starlette() + request = Request( + { + "type": "http", + "http_version": "1.1", + "method": "GET", + "path": "/boom", + "raw_path": b"/boom", + "query_string": b"", + "scheme": "http", + "server": ("testserver", 80), + "client": ("testclient", 1234), + "headers": [], + "app": app, + } + ) + + resp = await render_error_page(request, 500, "kaboom") + + assert resp.status_code == 500 + assert b"kaboom" in resp.body diff --git a/framework/hosting/tests/test_maintenance_mode.py b/framework/hosting/tests/test_maintenance_mode.py new file mode 100644 index 00000000..f60e8ac4 --- /dev/null +++ b/framework/hosting/tests/test_maintenance_mode.py @@ -0,0 +1,179 @@ +"""Maintenance mode gates everyone except the people who can turn it off.""" + +from __future__ import annotations + +import httpx +import pytest +from simple_module_hosting.maintenance import MaintenanceMiddleware +from starlette.applications import Starlette +from starlette.responses import PlainTextResponse +from starlette.routing import Route + + +class _User: + def __init__(self, roles: list[str]) -> None: + self.roles = roles + + +class _Provider: + """Mirrors the AuthProvider surface MaintenanceMiddleware actually uses.""" + + def __init__(self, *, explode: bool = False) -> None: + self._explode = explode + + def get_public_paths(self): + if self._explode: + raise RuntimeError("provider is down") + return (("/users/login",), ("/exact-public",)) + + +def _build_app( + *, + enabled: bool, + message: str = "", + user: _User | None = None, + provider: _Provider | None = _Provider(), +) -> Starlette: + async def ok(request): + return PlainTextResponse("app reached") + + app = Starlette( + routes=[ + Route("/protected", ok), + Route("/users/login", ok), + Route("/exact-public", ok), + Route("/health", ok), + Route("/api/thing", ok), + ] + ) + + class _HostState: + class settings: + maintenance_mode = enabled + maintenance_message = message + + app.state.host = _HostState() + + class _AuthState: + auth_provider = provider + + app.state.auth = _AuthState() + + app.add_middleware(MaintenanceMiddleware) + # Stands in for AuthMiddleware, which runs further out and is what puts the + # resolved user on request.state. Added last so it executes first, exactly + # as the real pipeline orders them. + app.add_middleware(_SeedUser, user=user) + return app + + +class _SeedUser: + """Minimal stand-in for AuthMiddleware's contribution to request.state.""" + + def __init__(self, app, user: _User | None) -> None: + self.app = app + self.user = user + + async def __call__(self, scope, receive, send) -> None: + if scope["type"] == "http" and self.user is not None: + scope.setdefault("state", {})["user"] = self.user + await self.app(scope, receive, send) + + +async def _get(app, path: str, **kwargs) -> httpx.Response: + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as c: + return await c.get(path, **kwargs) + + +class TestGateClosed: + async def test_anonymous_visitor_gets_503(self) -> None: + resp = await _get(_build_app(enabled=True), "/protected") + assert resp.status_code == 503 + + async def test_non_admin_gets_503(self) -> None: + app = _build_app(enabled=True, user=_User(["editor"])) + resp = await _get(app, "/protected") + assert resp.status_code == 503 + + async def test_admin_passes_through(self) -> None: + """Someone has to be able to reach settings and switch it back off.""" + app = _build_app(enabled=True, user=_User(["admin"])) + resp = await _get(app, "/protected") + assert resp.status_code == 200 + assert resp.text == "app reached" + + async def test_retry_after_is_advertised(self) -> None: + resp = await _get(_build_app(enabled=True), "/api/thing") + assert resp.headers.get("Retry-After") + + +class TestAlwaysReachable: + async def test_health_probe_survives(self) -> None: + """Orchestrators must not kill the pod mid-maintenance.""" + resp = await _get(_build_app(enabled=True), "/health") + assert resp.status_code == 200 + + async def test_login_prefix_stays_open(self) -> None: + """An admin signed out when the switch flipped must still get in.""" + resp = await _get(_build_app(enabled=True), "/users/login") + assert resp.status_code == 200 + + async def test_exact_public_path_stays_open(self) -> None: + resp = await _get(_build_app(enabled=True), "/exact-public") + assert resp.status_code == 200 + + +class TestGateOpen: + async def test_disabled_is_a_no_op(self) -> None: + resp = await _get(_build_app(enabled=False), "/protected") + assert resp.status_code == 200 + + async def test_anonymous_reaches_app_when_disabled(self) -> None: + resp = await _get(_build_app(enabled=False, user=None), "/protected") + assert resp.text == "app reached" + + +class TestDegradedDependencies: + async def test_missing_host_state_does_not_block_traffic(self) -> None: + """Fail open on a missing setting — a config gap must not take the + site down, which is the exact failure this feature would cause.""" + + async def ok(request): + return PlainTextResponse("app reached") + + app = Starlette(routes=[Route("/protected", ok)]) + app.add_middleware(MaintenanceMiddleware) + resp = await _get(app, "/protected") + assert resp.status_code == 200 + + async def test_broken_provider_still_gates(self) -> None: + """A provider that raises must not accidentally open the gate.""" + app = _build_app(enabled=True, provider=_Provider(explode=True)) + resp = await _get(app, "/protected") + assert resp.status_code == 503 + + async def test_no_auth_provider_still_gates(self) -> None: + app = _build_app(enabled=True, provider=None) + resp = await _get(app, "/protected") + assert resp.status_code == 503 + + +class TestJsonCallers: + async def test_api_caller_gets_json_not_html(self) -> None: + app = _build_app(enabled=True, message="Back at 14:00 UTC") + resp = await _get(app, "/api/thing") + assert resp.status_code == 503 + assert resp.json()["detail"] == "Back at 14:00 UTC" + + async def test_generic_detail_when_no_message_set(self) -> None: + resp = await _get(_build_app(enabled=True), "/api/thing") + assert resp.json()["detail"] + + +@pytest.mark.parametrize("roles", [[], ["viewer"], ["admin-ish"], ["Admin"]]) +async def test_only_the_exact_admin_role_bypasses(roles: list[str]) -> None: + """Substring or case-insensitive matching here would be a privilege bug.""" + app = _build_app(enabled=True, user=_User(roles)) + resp = await _get(app, "/protected") + assert resp.status_code == 503 diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py index 0410ec7f..096c2675 100644 --- a/framework/hosting/tests/test_middleware_order.py +++ b/framework/hosting/tests/test_middleware_order.py @@ -4,7 +4,7 @@ CorrelationId → RequestLogging → GZip → Security → Session → → Tenant (opt-in) → Locale → InertiaLayoutData - → InertiaCache → CommitBeforeResponse → app + → InertiaCache → Maintenance → CommitBeforeResponse → app InertiaCache sits directly inside InertiaLayoutData, the middleware that puts this user's auth block, permissions and menus into every Inertia payload. Being @@ -12,6 +12,12 @@ read the headers, and pairing the two keeps "what makes the payload per-user" and "what stops the payload being cached" from drifting apart. +Maintenance sits inside InertiaCache rather than outside it. Its 503 is an +Inertia payload carrying the same per-user auth block and menus, and it is +produced by short-circuiting — so if it sat outside, that payload would never +pass through the cache guard and would ship storable, which is the exact bug +InertiaCache exists to prevent. + Tenant/Locale must see ``request.state.user`` set by AuthMiddleware so DB queries get filtered correctly; CorrelationId must wrap everything so every log line carries its id. SiteLock must precede AuthMiddleware: it gates @@ -20,6 +26,12 @@ to be fully hidden. That inversion breaks the feature without failing any site_lock unit test, which is why the order is pinned here. +Maintenance sits after InertiaLayoutData because its 503 page renders +through Inertia and needs the shared props (auth, menus, i18n) — placed any +further out it would render bare, with no layout and untranslated copy. It is +also after Auth, which is what tells it whether the caller is an admin allowed +to pass through and switch it back off. + CommitBeforeResponse is innermost so its send-wrapper is the first to see ``http.response.start`` — that is what makes the request's DB work commit before any byte reaches the client (GH #257). Anything added inside it would @@ -52,6 +64,7 @@ "LocaleMiddleware", "InertiaLayoutDataMiddleware", "InertiaCacheMiddleware", + "MaintenanceMiddleware", "CommitBeforeResponseMiddleware", ) @@ -66,6 +79,7 @@ "LocaleMiddleware", "InertiaLayoutDataMiddleware", "InertiaCacheMiddleware", + "MaintenanceMiddleware", "CommitBeforeResponseMiddleware", ) diff --git a/host/client_app/app.tsx b/host/client_app/app.tsx index 04dbe279..a9ef8c91 100644 --- a/host/client_app/app.tsx +++ b/host/client_app/app.tsx @@ -1,5 +1,6 @@ import { createInertiaApp, router } from '@inertiajs/react'; import { ErrorBoundary } from '@simple-module-py/ui/components/ErrorBoundary'; +import { OfflineBanner } from '@simple-module-py/ui/components/OfflineBanner'; import { formatTitle, setTitleAppName } from '@simple-module-py/ui/lib/app-title'; import { startSpaLinkInterception } from '@simple-module-py/ui/lib/spa-links'; import { useEffect, useRef } from 'react'; @@ -43,6 +44,10 @@ createInertiaApp({ return ( + {/* Outside the page, so connectivity is reported on the error and + auth screens too — losing the network on the login page is when + an unexplained failure is most confusing. */} + ); diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index 927e5b4d..1f3d3bb0 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -3,46 +3,103 @@ import { keys, useT } from '@simple-module-py/i18n'; import { CopyableId } from '@simple-module-py/ui/components/CopyableId'; import { ErrorScreen } from '@simple-module-py/ui/components/ErrorScreen'; import { Button } from '@simple-module-py/ui/components/ui/button'; -import { Home, LifeBuoy } from 'lucide-react'; +import { Home, LifeBuoy, LogIn } from 'lucide-react'; interface Props { status: number; message: string; correlation_id?: string; + /** Provider-specific login URL, when an auth provider is installed. Only + * used by the statuses where signing in is the actual remedy. */ + login_url?: string | null; } -function ErrorPage({ status, message, correlation_id }: Props) { +type Accent = 'primary' | 'warning' | 'destructive'; + +interface StatusCopy { + title: string; + description: string; + accent: Accent; +} + +/** One row per status, rather than three parallel Record maps — + * those drift the moment a status is added to one and missed in another. */ +function useStatusCopy(status: number): StatusCopy { const { t } = useT(); + const e = keys.host.error; - const titles: Record = { - 403: t(keys.host.error.forbidden_title), - 404: t(keys.host.error.not_found_title), - 500: t(keys.host.error.server_error_title), + const table: Record = { + 401: { + title: t(e.unauthorized_title), + description: t(e.unauthorized_description), + accent: 'warning', + }, + 403: { + title: t(e.forbidden_title), + description: t(e.forbidden_description), + accent: 'warning', + }, + 404: { + title: t(e.not_found_title), + description: t(e.not_found_description), + accent: 'primary', + }, + 419: { + title: t(e.session_expired_title), + description: t(e.session_expired_description), + accent: 'warning', + }, + 422: { + title: t(e.invalid_request_title), + description: t(e.invalid_request_description), + accent: 'warning', + }, + 429: { + title: t(e.rate_limited_title), + description: t(e.rate_limited_description), + accent: 'warning', + }, + 500: { + title: t(e.server_error_title), + description: t(e.server_error_description), + accent: 'destructive', + }, + 503: { + title: t(e.unavailable_title), + description: t(e.unavailable_description), + accent: 'destructive', + }, }; - const descriptions: Record = { - 403: t(keys.host.error.forbidden_description), - 404: t(keys.host.error.not_found_description), - 500: t(keys.host.error.server_error_description), - }; + return ( + table[status] ?? { + title: t(e.generic_title), + description: t(e.generic_description), + accent: 'primary', + } + ); +} - const accents: Record = { - 403: 'warning', - 404: 'primary', - 500: 'destructive', - }; +/** Statuses where "sign in" is the remedy, not "go home". */ +const SIGN_IN_STATUSES = new Set([401, 419]); + +function ErrorPage({ status, message, correlation_id, login_url }: Props) { + const { t } = useT(); + const copy = useStatusCopy(status); - const title = titles[status] || t(keys.host.error.generic_title); - const description = message || descriptions[status] || t(keys.host.error.generic_description); + // A server-supplied message wins over the canned description — it is the + // specific reason, where the table only knows the status class. + const description = message || copy.description; + const showSignIn = SIGN_IN_STATUSES.has(status) && Boolean(login_url); return ( <> - + @@ -58,7 +115,15 @@ function ErrorPage({ status, message, correlation_id }: Props) { ) : undefined } > - + )} + } @@ -196,5 +196,5 @@ function RoleEdit({ role, assigned, groups }: Props) { ); } -RoleEdit.layout = (page: React.ReactNode) => {page}; +RoleEdit.layout = (page: React.ReactNode) => {page}; export default RoleEdit; diff --git a/modules/permissions/permissions/pages/UserEdit.tsx b/modules/permissions/permissions/pages/UserEdit.tsx index 0efaaa58..24ac2303 100644 --- a/modules/permissions/permissions/pages/UserEdit.tsx +++ b/modules/permissions/permissions/pages/UserEdit.tsx @@ -6,7 +6,7 @@ import { Badge } from '@simple-module-py/ui/components/ui/badge'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { USERS_ADMIN_PATH } from '@simple-module-py/ui/lib/auth-routes'; import { Check, KeyRound, Link2, Package, Search, ShieldCheck } from 'lucide-react'; import type React from 'react'; @@ -200,5 +200,5 @@ function UserEdit({ user, roles, direct, inherited, inherited_by: inheritedBy, g ); } -UserEdit.layout = (page: React.ReactNode) => {page}; +UserEdit.layout = (page: React.ReactNode) => {page}; export default UserEdit; diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index 69dc9cf1..8a213462 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -44,7 +44,7 @@ # ── Routing ────────────────────────────────────────────────────────── API_PREFIX: Final = "/api/settings" -VIEW_PREFIX: Final = "/settings" +VIEW_PREFIX: Final = "/admin/settings" VIEW_CREATE_PATH: Final = "/create" VIEW_EDIT_PATH: Final = "/{setting_id}/edit" VIEW_MODULES_PATH: Final = "/modules" diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 7f0ebcf8..3509b078 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -38,6 +38,7 @@ VIEW_CREATE_PATH, VIEW_EDIT_PATH, VIEW_MODULES_PATH, + VIEW_PREFIX, VIEW_STORE_PATH, ) from settings.contracts.schemas import SettingCreate, SettingUpdate @@ -50,9 +51,11 @@ _PAGE_MODULES_EDIT = "Settings/ModulesEdit" # Row-level actions return to the raw store they were performed in, not to -# the module forms that now own the section root. -_REDIRECT_SETTINGS = "/settings/store" -_REDIRECT_MODULES = "/settings/" +# the module forms that now own the section root. Built from VIEW_PREFIX so +# they follow the section if it moves again — spelled out, they silently sent +# users to the pre-/admin paths after the move. +_REDIRECT_SETTINGS = f"{VIEW_PREFIX}{VIEW_STORE_PATH}" +_REDIRECT_MODULES = f"{VIEW_PREFIX}/" # Every screen in this section reads configuration: module field values, # their env var names, and now which of the two is in force. The matching JSON diff --git a/modules/settings/settings/module.py b/modules/settings/settings/module.py index c9e64187..45f73dc4 100644 --- a/modules/settings/settings/module.py +++ b/modules/settings/settings/module.py @@ -67,7 +67,7 @@ def register_menu_items(self, registry: MenuRegistry) -> None: url=MENU_URL, icon=MENU_ICON, order=MENU_ORDER, - section=MenuSection.SIDEBAR, + section=MenuSection.ADMIN_SIDEBAR, group="System", # Mirrors the view router's guard, so the entry is not offered # to accounts whose click would 403. diff --git a/modules/settings/settings/pages/Browse.tsx b/modules/settings/settings/pages/Browse.tsx index d11e3944..694983d1 100644 --- a/modules/settings/settings/pages/Browse.tsx +++ b/modules/settings/settings/pages/Browse.tsx @@ -12,7 +12,7 @@ import { TableHeader, TableRow, } from '@simple-module-py/ui/components/ui/table'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { Box, Plus, Settings as SettingsIcon } from 'lucide-react'; import type React from 'react'; import type { ValueType } from './components/ValueInput'; @@ -153,5 +153,5 @@ function Browse({ settings }: Props) { ); } -Browse.layout = (page: React.ReactNode) => {page}; +Browse.layout = (page: React.ReactNode) => {page}; export default Browse; diff --git a/modules/settings/settings/pages/Create.tsx b/modules/settings/settings/pages/Create.tsx index 3e553961..31d6a334 100644 --- a/modules/settings/settings/pages/Create.tsx +++ b/modules/settings/settings/pages/Create.tsx @@ -13,7 +13,7 @@ import { SelectValue, } from '@simple-module-py/ui/components/ui/select'; import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import type React from 'react'; import { KeyField, type KnownKey } from './components/KeyField'; import ValueInput, { VALUE_TYPES, type ValueType } from './components/ValueInput'; @@ -160,5 +160,5 @@ function Create({ known_keys }: Props) { ); } -Create.layout = (page: React.ReactNode) => {page}; +Create.layout = (page: React.ReactNode) => {page}; export default Create; diff --git a/modules/settings/settings/pages/Edit.tsx b/modules/settings/settings/pages/Edit.tsx index edbfadae..64a25418 100644 --- a/modules/settings/settings/pages/Edit.tsx +++ b/modules/settings/settings/pages/Edit.tsx @@ -6,7 +6,7 @@ import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; import { Label } from '@simple-module-py/ui/components/ui/label'; import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import type React from 'react'; import ValueInput, { type ValueType } from './components/ValueInput'; import { ROUTES } from './routes'; @@ -128,5 +128,5 @@ function Edit({ setting }: Props) { ); } -Edit.layout = (page: React.ReactNode) => {page}; +Edit.layout = (page: React.ReactNode) => {page}; export default Edit; diff --git a/modules/settings/settings/pages/ModulesEdit.tsx b/modules/settings/settings/pages/ModulesEdit.tsx index 144e1110..09fca6bd 100644 --- a/modules/settings/settings/pages/ModulesEdit.tsx +++ b/modules/settings/settings/pages/ModulesEdit.tsx @@ -2,7 +2,7 @@ import { Head } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { Card } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { Box, Search } from 'lucide-react'; import type React from 'react'; import { useMemo, useState } from 'react'; @@ -109,5 +109,5 @@ function ModulesEdit({ modules, testable = [] }: Props) { ); } -ModulesEdit.layout = (page: React.ReactNode) => {page}; +ModulesEdit.layout = (page: React.ReactNode) => {page}; export default ModulesEdit; diff --git a/modules/settings/settings/pages/routes.ts b/modules/settings/settings/pages/routes.ts index 1c76cb1e..6b901088 100644 --- a/modules/settings/settings/pages/routes.ts +++ b/modules/settings/settings/pages/routes.ts @@ -1,10 +1,10 @@ export const ROUTES = { /** Per-module forms — the section root, and where "Settings" now lands. */ - modules: '/settings/', + modules: '/admin/settings/', /** Raw key/value store, demoted from the root. */ - browse: '/settings/store', - create: '/settings/create', - edit: (id: number) => `/settings/${id}/edit`, - byId: (id: number) => `/settings/${id}`, - testConnection: (pkg: string) => `/settings/test-connection/${pkg}`, + browse: '/admin/settings/store', + create: '/admin/settings/create', + edit: (id: number) => `/admin/settings/${id}/edit`, + byId: (id: number) => `/admin/settings/${id}`, + testConnection: (pkg: string) => `/admin/settings/test-connection/${pkg}`, } as const; diff --git a/modules/settings/tests/test_settings_field_sources.py b/modules/settings/tests/test_settings_field_sources.py index 0bcaaca5..e4888fcd 100644 --- a/modules/settings/tests/test_settings_field_sources.py +++ b/modules/settings/tests/test_settings_field_sources.py @@ -53,7 +53,7 @@ def test_db_override_beats_env(self): class TestModulesView: async def test_fields_carry_their_source(self, authenticated_client): - resp = await authenticated_client.get("/settings/", follow_redirects=False) + resp = await authenticated_client.get("/admin/settings/", follow_redirects=False) assert resp.status_code == 200 def test_env_var_presence_is_detected(self, monkeypatch: pytest.MonkeyPatch): @@ -102,18 +102,18 @@ def test_overrides_mark_their_fields(self): class TestTestConnectionEndpoint: async def test_unknown_package_is_a_404(self, authenticated_client): - resp = await authenticated_client.post("/settings/test-connection/nosuchmodule") + resp = await authenticated_client.post("/admin/settings/test-connection/nosuchmodule") assert resp.status_code == 404 async def test_module_without_checks_is_a_404(self, authenticated_client): """Only modules that registered a check can be tested.""" - resp = await authenticated_client.post("/settings/test-connection/settings") + resp = await authenticated_client.post("/admin/settings/test-connection/settings") assert resp.status_code == 404 async def test_failing_check_still_returns_200_with_the_reason(self, authenticated_client): """An admin testing a connection needs to read the failure, not get an error status with the reason buried.""" - resp = await authenticated_client.post("/settings/test-connection/file_storage") + resp = await authenticated_client.post("/admin/settings/test-connection/file_storage") assert resp.status_code == 200, resp.text body = resp.json() assert body["checks"], body diff --git a/modules/settings/tests/test_settings_view_authz.py b/modules/settings/tests/test_settings_view_authz.py index ccac1cbc..71e08332 100644 --- a/modules/settings/tests/test_settings_view_authz.py +++ b/modules/settings/tests/test_settings_view_authz.py @@ -12,7 +12,7 @@ import pytest from simple_module_test.fixtures import forge_session_cookie -_VIEW_ROUTES = ["/settings/", "/settings/store", "/settings/create"] +_VIEW_ROUTES = ["/admin/settings/", "/admin/settings/store", "/admin/settings/create"] @pytest.fixture @@ -50,7 +50,7 @@ async def test_view_routes_reject_a_user_without_settings_view( async def test_the_api_and_the_screen_agree(plain_user_client: httpx.AsyncClient): """Same data, same answer — the gap between them was the bug.""" api = await plain_user_client.get("/api/settings/modules", follow_redirects=False) - view = await plain_user_client.get("/settings/", follow_redirects=False) + view = await plain_user_client.get("/admin/settings/", follow_redirects=False) assert api.status_code in (302, 401, 403) assert view.status_code in (302, 401, 403) diff --git a/modules/users/tests/test_users_login_deep_link.py b/modules/users/tests/test_users_login_deep_link.py new file mode 100644 index 00000000..1a8f5f49 --- /dev/null +++ b/modules/users/tests/test_users_login_deep_link.py @@ -0,0 +1,63 @@ +"""An anonymous visit to a protected page should come back after login. + +AuthMiddleware stashes the target in the session; the login view surfaces it +as ``login_redirect_url``. Before this existed the target was dropped and +every login landed on the configured default. + +Filename is prefixed with the module name on purpose: no tests/ directory +here has an __init__.py, so test module basenames share one global namespace. +""" + +from __future__ import annotations + +import pytest + + +class TestDeepLinkAfterLogin: + """An anonymous visit to a protected page should come back after login. + + AuthMiddleware stashes the target in the session; the login view surfaces + it as ``login_redirect_url``. Before this existed the target was dropped + and every login landed on the configured default. + """ + + @pytest.mark.anyio + async def test_bounced_target_becomes_the_redirect_prop(self, anon_client): + bounced = await anon_client.get("/admin/settings/", follow_redirects=False) + assert bounced.status_code == 302 + + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/admin/settings/" + + @pytest.mark.anyio + async def test_query_string_is_preserved(self, anon_client): + await anon_client.get("/admin/settings/?tab=modules", follow_redirects=False) + + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/admin/settings/?tab=modules" + + @pytest.mark.anyio + async def test_reload_of_login_page_keeps_the_target(self, anon_client): + """Read-not-pop: reloading the login page must not lose the deep link.""" + await anon_client.get("/admin/settings/", follow_redirects=False) + + for _ in range(2): + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/admin/settings/" + + @pytest.mark.anyio + async def test_without_a_bounce_the_default_is_used(self, anon_client): + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/dashboard/" diff --git a/modules/users/tests/test_views.py b/modules/users/tests/test_views.py index a4af5f2e..8cb4bdec 100644 --- a/modules/users/tests/test_views.py +++ b/modules/users/tests/test_views.py @@ -177,19 +177,19 @@ class TestAdminIndexPage: @pytest.mark.anyio async def test_admin_without_auth_is_redirected(self, anon_client): """Unauthenticated access to the admin page redirects to /users/login.""" - resp = await anon_client.get("/users/admin", follow_redirects=False) + resp = await anon_client.get("/admin/users", follow_redirects=False) assert resp.status_code == 302 assert resp.headers["location"].endswith("/users/login") @pytest.mark.anyio async def test_admin_with_admin_session_returns_200(self, admin_client): - resp = await admin_client.get("/users/admin") + resp = await admin_client.get("/admin/users/") assert resp.status_code == 200 @pytest.mark.anyio async def test_admin_inertia_component_is_users_index(self, admin_client): resp = await admin_client.get( - "/users/admin", + "/admin/users/", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) assert resp.status_code == 200 @@ -200,26 +200,26 @@ async def test_admin_inertia_component_is_users_index(self, admin_client): class TestAdminEditPage: @pytest.mark.anyio async def test_invalid_uuid_returns_404(self, admin_client): - resp = await admin_client.get("/users/admin/not-a-uuid") + resp = await admin_client.get("/admin/users/not-a-uuid") assert resp.status_code == 404 @pytest.mark.anyio async def test_unknown_uuid_returns_404(self, admin_client): missing_id = str(uuid.uuid4()) - resp = await admin_client.get(f"/users/admin/{missing_id}") + resp = await admin_client.get(f"/admin/users/{missing_id}") assert resp.status_code == 404 @pytest.mark.anyio async def test_existing_user_returns_200(self, admin_client, users_db): user = await _make_verified_user(users_db, email="edit_target@example.com") - resp = await admin_client.get(f"/users/admin/{user.id}") + resp = await admin_client.get(f"/admin/users/{user.id}") assert resp.status_code == 200 @pytest.mark.anyio async def test_existing_user_inertia_component(self, admin_client, users_db): user = await _make_verified_user(users_db, email="edit_target2@example.com") resp = await admin_client.get( - f"/users/admin/{user.id}", + f"/admin/users/{user.id}", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) assert resp.status_code == 200 @@ -232,7 +232,7 @@ async def test_admin_edit_page_unknown_user_returns_404(admin_client): import uuid resp = await admin_client.get( - f"/users/admin/{uuid.uuid4()}", + f"/admin/users/{uuid.uuid4()}", follow_redirects=False, ) assert resp.status_code == 404 @@ -275,53 +275,3 @@ async def test_login_redirect_fallback_without_dashboard(users_app): assert url.endswith("/"), "must have trailing slash" finally: object.__setattr__(sm, "modules", original) - - -class TestDeepLinkAfterLogin: - """An anonymous visit to a protected page should come back after login. - - AuthMiddleware stashes the target in the session; the login view surfaces - it as ``login_redirect_url``. Before this existed the target was dropped - and every login landed on the configured default. - """ - - @pytest.mark.anyio - async def test_bounced_target_becomes_the_redirect_prop(self, anon_client): - bounced = await anon_client.get("/settings/", follow_redirects=False) - assert bounced.status_code == 302 - - resp = await anon_client.get( - "/users/login", - headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, - ) - assert resp.json()["props"]["login_redirect_url"] == "/settings/" - - @pytest.mark.anyio - async def test_query_string_is_preserved(self, anon_client): - await anon_client.get("/settings/?tab=modules", follow_redirects=False) - - resp = await anon_client.get( - "/users/login", - headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, - ) - assert resp.json()["props"]["login_redirect_url"] == "/settings/?tab=modules" - - @pytest.mark.anyio - async def test_reload_of_login_page_keeps_the_target(self, anon_client): - """Read-not-pop: reloading the login page must not lose the deep link.""" - await anon_client.get("/settings/", follow_redirects=False) - - for _ in range(2): - resp = await anon_client.get( - "/users/login", - headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, - ) - assert resp.json()["props"]["login_redirect_url"] == "/settings/" - - @pytest.mark.anyio - async def test_without_a_bounce_the_default_is_used(self, anon_client): - resp = await anon_client.get( - "/users/login", - headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, - ) - assert resp.json()["props"]["login_redirect_url"] == "/dashboard/" diff --git a/modules/users/tests/test_views_admin.py b/modules/users/tests/test_views_admin.py index 58d18fb5..bb5466b1 100644 --- a/modules/users/tests/test_views_admin.py +++ b/modules/users/tests/test_views_admin.py @@ -20,7 +20,7 @@ async def test_status_filter_in_view(self, admin_client, users_db): await users_db.commit() resp = await admin_client.get( - "/users/admin?status=disabled", + "/admin/users/?status=disabled", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -35,7 +35,7 @@ async def test_status_filter_in_view(self, admin_client, users_db): @pytest.mark.anyio async def test_filters_defaults_in_props(self, admin_client): resp = await admin_client.get( - "/users/admin", + "/admin/users/", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -50,7 +50,7 @@ async def test_filters_defaults_in_props(self, admin_client): @pytest.mark.anyio async def test_invalid_filter_values_are_ignored(self, admin_client): resp = await admin_client.get( - "/users/admin?status=bad&sort=invalid&order=sideways", + "/admin/users/?status=bad&sort=invalid&order=sideways", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -71,7 +71,7 @@ async def test_page_past_the_end_clamps_to_last_page(self, admin_client, users_d await _make_user(users_db, email="clamp-b@x.com") resp = await admin_client.get( - "/users/admin?page=999&per_page=1", + "/admin/users/?page=999&per_page=1", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -90,7 +90,7 @@ async def test_pagination_prop_echoes_clamped_values(self, admin_client, users_d await _make_user(users_db, email="clamp-c@x.com") resp = await admin_client.get( - "/users/admin?page=0&per_page=1000", + "/admin/users/?page=0&per_page=1000", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -131,7 +131,7 @@ async def test_flag_true_when_permissions_installed(self, admin_client, users_ap ) try: resp = await admin_client.get( - f"/users/admin/{user.id}", + f"/admin/users/{user.id}", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) finally: @@ -156,7 +156,7 @@ async def test_flag_false_when_not_installed(self, admin_client, users_app, user ) try: resp = await admin_client.get( - f"/users/admin/{user.id}", + f"/admin/users/{user.id}", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) finally: @@ -176,7 +176,7 @@ class TestAdminAddPeoplePage: @pytest.mark.anyio async def test_add_page_renders_with_roles(self, admin_client): resp = await admin_client.get( - "/users/admin/add", + "/admin/users/add", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -188,7 +188,7 @@ async def test_add_page_renders_with_roles(self, admin_client): async def test_add_page_reports_whether_mail_can_be_delivered(self, admin_client): """Drives the copy-link panel — the page has to know before submitting.""" resp = await admin_client.get( - "/users/admin/add", + "/admin/users/add", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert "mailer_delivers" in resp.json()["props"] @@ -201,7 +201,7 @@ async def test_no_mailer_does_not_promise_delivery(self, admin_client, app): app.state.users.mailer = None try: resp = await admin_client.get( - "/users/admin/add", + "/admin/users/add", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.json()["props"]["mailer_delivers"] is False @@ -210,22 +210,22 @@ async def test_no_mailer_does_not_promise_delivery(self, admin_client, app): @pytest.mark.anyio async def test_add_page_requires_auth(self, anon_client): - resp = await anon_client.get("/users/admin/add", follow_redirects=False) + resp = await anon_client.get("/admin/users/add", follow_redirects=False) assert resp.status_code == 302 @pytest.mark.anyio @pytest.mark.parametrize( ("old_path", "mode"), - [("/users/admin/create", "create"), ("/users/admin/invite", "invite")], + [("/admin/users/create", "create"), ("/admin/users/invite", "invite")], ) async def test_old_urls_redirect_into_the_right_mode(self, admin_client, old_path, mode): """Existing links must land on the merged form with their mode preselected.""" resp = await admin_client.get(old_path, follow_redirects=False) assert resp.status_code == 307 - assert resp.headers["location"] == f"/users/admin/add?mode={mode}" + assert resp.headers["location"] == f"/admin/users/add?mode={mode}" @pytest.mark.anyio - @pytest.mark.parametrize("old_path", ["/users/admin/create", "/users/admin/invite"]) + @pytest.mark.parametrize("old_path", ["/admin/users/create", "/admin/users/invite"]) async def test_old_urls_require_auth(self, anon_client, old_path): """The legacy aliases must stay gated behind the same permission as the page they redirect to — an anonymous caller must not reach the diff --git a/modules/users/users/admin/components/UserRow.tsx b/modules/users/users/admin/components/UserRow.tsx index c74ae03d..2a8c45f9 100644 --- a/modules/users/users/admin/components/UserRow.tsx +++ b/modules/users/users/admin/components/UserRow.tsx @@ -78,7 +78,7 @@ export function UserRow({ user }: { user: UserListItem }) { diff --git a/modules/users/users/admin/components/UsersEmpty.tsx b/modules/users/users/admin/components/UsersEmpty.tsx index 979538ea..cb604fef 100644 --- a/modules/users/users/admin/components/UsersEmpty.tsx +++ b/modules/users/users/admin/components/UsersEmpty.tsx @@ -5,7 +5,7 @@ import { TableEmptyRow } from '@simple-module-py/ui/components/TableEmptyRow'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Plus, UserPlus, Users } from 'lucide-react'; -const ADD_PEOPLE_URL = '/users/admin/add'; +const ADD_PEOPLE_URL = '/admin/users/add'; function AddPeopleAction() { const { t } = useT(); diff --git a/modules/users/users/admin/views.py b/modules/users/users/admin/views.py index 8da0d24b..e0e9dc62 100644 --- a/modules/users/users/admin/views.py +++ b/modules/users/users/admin/views.py @@ -30,7 +30,7 @@ async def _roles_payload(app) -> list[dict[str, str]]: @router.get( - "/admin", + "/", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) @@ -96,7 +96,7 @@ async def admin_index( @router.get( - "/admin/add", + "/add", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) @@ -126,27 +126,27 @@ async def admin_add_people_page( @router.get( - "/admin/invite", + "/invite", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) async def admin_invite_redirect() -> RedirectResponse: """Old invite URL — the flow merged into /users/admin/add.""" - return RedirectResponse("/users/admin/add?mode=invite", status_code=307) + return RedirectResponse("/admin/users/add?mode=invite", status_code=307) @router.get( - "/admin/create", + "/create", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) async def admin_create_redirect() -> RedirectResponse: """Old create URL — the flow merged into /users/admin/add.""" - return RedirectResponse("/users/admin/add?mode=create", status_code=307) + return RedirectResponse("/admin/users/add?mode=create", status_code=307) @router.get( - "/admin/{user_id}", + "/{user_id}", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index f027d405..a2f26b96 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -15,10 +15,9 @@ from fastapi import APIRouter, Depends, HTTPException, Request, Response, status from fastapi.security import OAuth2PasswordRequestForm from fastapi_users import exceptions as fu_exceptions - -from users.auth_local.rate_limit import LoginRateLimiter, ThroughputLimiter from simple_module_core.redirect_safety import SESSION_NEXT_KEY +from users.auth_local.rate_limit import LoginRateLimiter, ThroughputLimiter from users.constants import SESSION_USER_ID_KEY from users.contracts.schemas import ( AcceptInviteRequest, diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 94fbd1c4..4d3ea77a 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -29,7 +29,7 @@ _MODULE_DEPENDENCY_SETTINGS = "Settings" # Menu URLs -_URL_USERS_ADMIN = "/users/admin" +_URL_USERS_ADMIN = "/admin/users/" _URL_USERS_ME = "/users/me" _URL_USERS_LOGOUT = "/users/logout" @@ -44,6 +44,10 @@ class UsersModule(ModuleBase): name="Users", route_prefix="/api/users", view_prefix="/users", + # Sign-in and self-service stay on /users; the management CRUD + # belongs with the other admin screens. One view_prefix cannot + # express both, hence the second router. + admin_view_prefix="/admin/users", depends_on=[_MODULE_DEPENDENCY_AUTH, _MODULE_DEPENDENCY_SETTINGS], ) _is_auth_provider = True @@ -116,7 +120,7 @@ def register_audit_links(self, registry: AuditLinkRegistry) -> None: # The model class name — what snapshot_changes records. Keying # this off __tablename__ ("users_user") silently never matches. entity_type=User.__name__, - url_template=f"{_URL_USERS_ADMIN}/{{id}}", + url_template=f"{_URL_USERS_ADMIN}{{id}}", label="User", ) ) @@ -129,9 +133,9 @@ def register_menu_items(self, registry: MenuRegistry) -> None: url=_URL_USERS_ADMIN, icon=_ICON_USERS, order=100, - section=MenuSection.SIDEBAR, + section=MenuSection.ADMIN_SIDEBAR, roles=[ADMIN_ROLE_NAME], - group="Administration", + group="Access", ) ) # Self-service: profile + logout live in the user dropdown. @@ -161,7 +165,6 @@ def locale_dirs(self) -> dict[str, Path]: def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from users.admin.api import admin_router - from users.admin.views import router as admin_views from users.auth_local import api as auth_local_api from users.auth_local.token_api import router as token_router from users.auth_local.views import router as auth_views @@ -198,7 +201,11 @@ def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None register_oauth_routes(api_router) view_router.include_router(auth_views) - view_router.include_router(admin_views) + + def register_admin_routes(self, admin_router: APIRouter) -> None: + from users.admin.views import router as admin_views + + admin_router.include_router(admin_views) async def on_startup(self, app: FastAPI) -> None: """Build the mailer, rate limiter, and apply production cookie params.""" diff --git a/modules/users/users/pages/Users/AddPeople.tsx b/modules/users/users/pages/Users/AddPeople.tsx index 4816d85e..c178715c 100644 --- a/modules/users/users/pages/Users/AddPeople.tsx +++ b/modules/users/users/pages/Users/AddPeople.tsx @@ -2,7 +2,7 @@ import { Link, router, usePage } from '@inertiajs/react'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import type React from 'react'; import { useState } from 'react'; import { toast } from 'sonner'; @@ -19,7 +19,7 @@ interface Props { mailer_delivers: boolean; } -const USERS_INDEX = '/users/admin'; +const USERS_INDEX = '/admin/users/'; function initialMode(): Mode { if (typeof window === 'undefined') return 'invite'; @@ -208,5 +208,5 @@ function AddPeople() { ); } -AddPeople.layout = (page: React.ReactNode) => {page}; +AddPeople.layout = (page: React.ReactNode) => {page}; export default AddPeople; diff --git a/modules/users/users/pages/Users/Edit.tsx b/modules/users/users/pages/Users/Edit.tsx index cd8d1786..d6d168a1 100644 --- a/modules/users/users/pages/Users/Edit.tsx +++ b/modules/users/users/pages/Users/Edit.tsx @@ -1,7 +1,7 @@ import { Link, router, usePage } from '@inertiajs/react'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Button } from '@simple-module-py/ui/components/ui/button'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import type React from 'react'; import { useEffect, useMemo, useRef, useState } from 'react'; import { toast } from 'sonner'; @@ -231,7 +231,7 @@ function Edit() {
{dirty && Unsaved changes} {/* Back to what is persisted, not to what the page loaded with — discarding must not visually undo a section that already saved. */} @@ -291,5 +291,5 @@ function Edit() { ); } -Edit.layout = (page: React.ReactNode) => {page}; +Edit.layout = (page: React.ReactNode) => {page}; export default Edit; diff --git a/modules/users/users/pages/Users/Index.tsx b/modules/users/users/pages/Users/Index.tsx index 89b71ca9..eb30391b 100644 --- a/modules/users/users/pages/Users/Index.tsx +++ b/modules/users/users/pages/Users/Index.tsx @@ -13,7 +13,7 @@ import { TableRow, } from '@simple-module-py/ui/components/ui/table'; import { Tabs, TabsContent, TabsList, TabsTrigger } from '@simple-module-py/ui/components/ui/tabs'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { ArrowDown, ArrowUp, @@ -105,7 +105,7 @@ function Index() { if (sort !== 'email') params.sort = sort; if (order !== 'asc') params.order = order; if (page > 1) params.page = String(page); - router.get('/users/admin', params, { preserveState: true, preserveScroll: true }); + router.get('/admin/users/', params, { preserveState: true, preserveScroll: true }); }, [search, filters], ); @@ -153,7 +153,7 @@ function Index() { // One entry point: invite-vs-create is a choice inside the form, not // a choice between two buttons made before seeing either. } diff --git a/modules/permissions/permissions/pages/UserEdit.tsx b/modules/permissions/permissions/pages/UserEdit.tsx index 24ac2303..9bff53a5 100644 --- a/modules/permissions/permissions/pages/UserEdit.tsx +++ b/modules/permissions/permissions/pages/UserEdit.tsx @@ -58,7 +58,7 @@ function UserEdit({ user, roles, direct, inherited, inherited_by: inheritedBy, g function handleSubmit(e: React.FormEvent) { e.preventDefault(); - put(`/permissions/users/${user.id}`, { + put(`/admin/permissions/users/${user.id}`, { preserveScroll: true, onSuccess: () => toast.success(t(keys.permissions.toasts.saved)), onError: () => toast.error(t(keys.permissions.toasts.save_failed)), From 5a69b404f02c8f57343eaffb55ee988d3b9e2cf9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 15:19:04 +0200 Subject: [PATCH 07/17] fix(admin): admit anyone with an admin sidebar entry to /admin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- host/routes.py | 26 ++++- host/tests/test_admin_overview_access.py | 121 +++++++++++++++++++++++ 2 files changed, 146 insertions(+), 1 deletion(-) create mode 100644 host/tests/test_admin_overview_access.py diff --git a/host/routes.py b/host/routes.py index c76427e1..b786ed0b 100644 --- a/host/routes.py +++ b/host/routes.py @@ -45,8 +45,32 @@ async def admin_overview(request: Request, inertia: InertiaDep) -> InertiaRespon AuthMiddleware has already rejected anonymous visitors by this point; the check below is the authorisation half, which it does not do. + + Admission mirrors what the page renders: anyone with at least one entry in + the admin sidebar can open it. Gating on the ``admin`` role alone was + inconsistent — the sidebar is filtered by roles *and* permissions, so a + custom role holding, say, ``settings.view`` reaches ``/admin/settings/``, + sees the AdminLayout badge pointing here, and would be met with a 403 for + a page that would have listed the one tool they can use. + + Admins are admitted regardless, so an install with no admin modules still + reaches the page's own empty state rather than a 403. """ user = getattr(request.state, "user", None) - if user is None or not is_admin(getattr(user, "roles", None)): + if user is None: + raise HTTPException(status_code=403, detail="Administrator access required") + if not (is_admin(getattr(user, "roles", None)) or _has_admin_entries(request)): raise HTTPException(status_code=403, detail="Administrator access required") return await inertia.render("Admin", {}) + + +def _has_admin_entries(request: Request) -> bool: + """Whether the viewer has any admin sidebar entry. + + Read from the shared props ``InertiaLayoutDataMiddleware`` already built + for this request, so admission and rendering cannot disagree — recomputing + the filter here is how the two drift apart. + """ + shared = getattr(request.state, "inertia_shared", None) or {} + menus = shared.get("menus") or {} + return bool(menus.get("adminSidebar")) diff --git a/host/tests/test_admin_overview_access.py b/host/tests/test_admin_overview_access.py new file mode 100644 index 00000000..ab374829 --- /dev/null +++ b/host/tests/test_admin_overview_access.py @@ -0,0 +1,121 @@ +"""Who may open the ``/admin`` overview. + +The page lists whatever is in the viewer's ``adminSidebar``, which the menu +registry filters by roles *and* permissions. Admission has to follow the same +rule: gating on the ``admin`` role alone meant a custom role holding a single +admin permission could reach the screen that permission unlocks, see the +AdminLayout badge pointing at ``/admin``, and get a 403 from a page that would +have listed exactly the one tool they can use. +""" + +from __future__ import annotations + +import httpx +import pytest +from simple_module_test.fixtures import forge_session_cookie + + +@pytest.fixture +def host_app(app): + """The framework fixture builds ``create_app()`` only — host-level routes + live in ``host/main.py`` and are otherwise absent from tests, so ``/admin`` + would 404 rather than exercise its guard. Mounting here keeps the real + middleware stack (auth, shared props) around the route. The ``app`` fixture + is function-scoped, so this does not leak into other tests.""" + from host.routes import router as host_router + + app.include_router(host_router) + return app + + +async def _client_for(app, *, email: str, roles: list[str], permissions: list[str]): + """Sign in an account holding exactly these roles and permissions.""" + from users.models import User, UserRole + from users.models.role import Role + + async with app.state.sm.db.session_factory() as session: + user = User(email=email, hashed_password="x", is_active=True, is_verified=True) + session.add(user) + await session.flush() + for role_name in roles: + role = Role(name=role_name) + session.add(role) + await session.flush() + session.add(UserRole(user_id=user.id, role_id=role.id)) + user_id = str(user.id) + await session.commit() + + if permissions: + registry = app.state.sm.permissions + for role_name in roles: + registry.map_role(role_name, permissions) + + signed = forge_session_cookie(app.state.sm.settings.secret_key, {"user_id": user_id}) + transport = httpx.ASGITransport(app=app) + return httpx.AsyncClient( + transport=transport, base_url="http://testserver", cookies={"session": signed} + ) + + +@pytest.mark.anyio +async def test_admin_reaches_the_overview(host_app) -> None: + client = await _client_for(host_app, email="root@example.com", roles=["admin"], permissions=[]) + async with client: + resp = await client.get("/admin", follow_redirects=False) + assert resp.status_code == 200, resp.text + + +@pytest.mark.anyio +async def test_admin_reaches_it_even_with_an_empty_sidebar(host_app) -> None: + """An install with no admin modules must still reach the page's own empty + state — 403'ing there would make that copy unreachable.""" + from simple_module_core.menu import MenuSection + + registry = host_app.state.sm.menu_registry + registry._items = [i for i in registry._items if i.section != MenuSection.ADMIN_SIDEBAR] + registry._sorted = None + client = await _client_for( + host_app, email="lonely-admin@example.com", roles=["admin"], permissions=[] + ) + async with client: + resp = await client.get("/admin", follow_redirects=False) + assert resp.status_code == 200, resp.text + + +@pytest.mark.anyio +async def test_account_with_no_admin_entries_is_refused(host_app) -> None: + """A plain signed-in user has an empty admin sidebar — nothing to show.""" + client = await _client_for(host_app, email="plain@example.com", roles=[], permissions=[]) + async with client: + resp = await client.get("/admin", follow_redirects=False) + assert resp.status_code == 403 + + +@pytest.mark.anyio +async def test_permission_holder_without_the_admin_role_is_admitted(host_app) -> None: + """The case the role-only gate got wrong. + + This account can open ``/admin/settings/`` on its ``settings.view`` grant, + so refusing it the overview that links there is self-contradictory. + """ + from settings.constants import PERM_VIEW + + client = await _client_for( + host_app, + email="settings-only@example.com", + roles=["settings-reader"], + permissions=[PERM_VIEW], + ) + async with client: + resp = await client.get("/admin", follow_redirects=False) + assert resp.status_code == 200, resp.text + + +@pytest.mark.anyio +async def test_anonymous_is_bounced_to_login(host_app) -> None: + """AuthMiddleware owns this half — a 403 here would leak that /admin exists.""" + transport = httpx.ASGITransport(app=host_app) + async with httpx.AsyncClient(transport=transport, base_url="http://testserver") as c: + resp = await c.get("/admin", follow_redirects=False) + assert resp.status_code == 302 + assert "/users/login" in resp.headers["location"] From 7397577e78a40196c1211cc35a401f7a1f2ff2f8 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 15:29:44 +0200 Subject: [PATCH 08/17] docs: fix stale admin-section menu metadata left over from the URL move 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 --- docs/modules/audit_log.md | 2 +- docs/modules/feature_flags.md | 2 +- docs/modules/settings.md | 16 ++++++++-------- docs/modules/users.md | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index 54ff08e0..c1963f99 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -120,7 +120,7 @@ There is no write permission — the trail is append-only and written by the fra | Label | URL | Icon | Section | Group | Order | |---|---|---|---|---|---| -| `Audit Log` | `/admin/audit-log` | `scroll-text` | `SIDEBAR` | `System` | `210` | +| `Audit Log` | `/admin/audit-log` | `scroll-text` | `ADMIN_SIDEBAR` | `System` | `210` | ## Inertia pages diff --git a/docs/modules/feature_flags.md b/docs/modules/feature_flags.md index 78183d3c..8fafab5f 100644 --- a/docs/modules/feature_flags.md +++ b/docs/modules/feature_flags.md @@ -113,7 +113,7 @@ Unique constraint on `(scope, scope_id, name)`. The `scope_id=""` (instead of `N | Label | URL | Icon | Section | Group | Order | |---|---|---|---|---|---| -| `Feature Flags` | `/admin/feature-flags` | `flag` | `SIDEBAR` | `Administration` | `110` | +| `Feature Flags` | `/admin/feature-flags` | `flag` | `ADMIN_SIDEBAR` | `System` | `110` | ## Inertia pages diff --git a/docs/modules/settings.md b/docs/modules/settings.md index 4dcf76e5..b63794aa 100644 --- a/docs/modules/settings.md +++ b/docs/modules/settings.md @@ -4,8 +4,8 @@ A DB-backed key/value store with system / tenant / user precedence, plus a per-m Two distinct surfaces: -1. **Generic key/value settings** — anything addressable by a string `key`. Useful for arbitrary config you don't want to wedge into a pydantic class. Edited at `/settings`. -2. **Per-module pydantic settings** — each module registers a `BaseSettings` subclass via `register_module_settings`. Hydrated from the DB at boot, edited at `/settings/modules`, hot-swapped on save with a `SettingsReloaded` event so dependents (SMTP clients, Celery configs, …) can rebuild. +1. **Generic key/value settings** — anything addressable by a string `key`. Useful for arbitrary config you don't want to wedge into a pydantic class. Edited at `/admin/settings/store`. +2. **Per-module pydantic settings** — each module registers a `BaseSettings` subclass via `register_module_settings`. Hydrated from the DB at boot, edited at `/admin/settings/`, hot-swapped on save with a `SettingsReloaded` event so dependents (SMTP clients, Celery configs, …) can rebuild. ## ModuleMeta @@ -100,11 +100,11 @@ All write endpoints require `settings.edit` / `settings.create` / `settings.dele | Method + path | Inertia component | |---|---| -| `GET /settings/` | `Settings/Browse` | -| `GET /settings/create` | `Settings/Create` | -| `GET /settings/{setting_id}/edit` | `Settings/Edit` | -| `GET /settings/modules` | `Settings/ModulesEdit` | -| `POST` / `PUT` / `DELETE /settings/...` | form actions; redirect to `/settings` | +| `GET /admin/settings/` | `Settings/Browse` | +| `GET /admin/settings/create` | `Settings/Create` | +| `GET /admin/settings/{setting_id}/edit` | `Settings/Edit` | +| `GET /admin/settings/modules` | legacy redirect → `/admin/settings/` | +| `POST` / `PUT` / `DELETE /admin/settings/...` | form actions; redirect to `/admin/settings` | ## Public contracts @@ -170,7 +170,7 @@ Unique constraint on `(scope, scope_id, key)`. | Label | URL | Icon | Section | Group | Order | |---|---|---|---|---|---| -| `Settings` | `/settings` | `settings` | `SIDEBAR` | `System` | `200` | +| `Settings` | `/admin/settings/` | `settings` | `ADMIN_SIDEBAR` | `System` | `200` | ## Events diff --git a/docs/modules/users.md b/docs/modules/users.md index ae1ee69b..97a55534 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -208,7 +208,7 @@ Everything else is DB-backed (initial values are pydantic defaults; edit at `/se | Label | URL | Icon | Section | Group | Order | Roles | |---|---|---|---|---|---|---| -| `Users` | `/admin/users/` | `users` | `SIDEBAR` | `Administration` | `100` | `["admin"]` | +| `Users` | `/admin/users/` | `users` | `ADMIN_SIDEBAR` | `Access` | `100` | `["admin"]` | | `Profile` | `/users/me` | `user` | `USER_DROPDOWN` | — | `990` | _logged-in_ | | `Logout` | `/users/logout` (POST) | `log-out` | `USER_DROPDOWN` | — | `999` | _logged-in_ | From 18f2fcbbc66422e26dc86d98e91a67cd742a18c2 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 15:37:24 +0200 Subject: [PATCH 09/17] test(maintenance): cover POST to a GET-only public route during maintenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../hosting/tests/test_maintenance_mode.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/framework/hosting/tests/test_maintenance_mode.py b/framework/hosting/tests/test_maintenance_mode.py index af010652..19325562 100644 --- a/framework/hosting/tests/test_maintenance_mode.py +++ b/framework/hosting/tests/test_maintenance_mode.py @@ -91,6 +91,12 @@ async def _get(app, path: str, **kwargs) -> httpx.Response: return await c.get(path, **kwargs) +async def _post(app, path: str, **kwargs) -> httpx.Response: + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as c: + return await c.post(path, **kwargs) + + class TestGateClosed: async def test_anonymous_visitor_gets_503(self) -> None: resp = await _get(_build_app(enabled=True), "/protected") @@ -201,7 +207,8 @@ async def test_module_public_route_stays_open(self) -> None: assert resp.status_code == 200 assert resp.text == "app reached" - async def test_method_not_covered_by_the_rule_still_gates(self) -> None: + async def test_uncovered_path_still_gates(self) -> None: + """A path with no matching rule at all is not exempted by an unrelated one.""" from simple_module_core.public_routes import PublicRouteRegistry registry = PublicRouteRegistry() @@ -210,6 +217,16 @@ async def test_method_not_covered_by_the_rule_still_gates(self) -> None: resp = await _get(app, "/api/thing") assert resp.status_code == 503 + async def test_wrong_method_on_a_covered_path_still_gates(self) -> None: + """The rule is GET-only — a POST to the same path must not bypass.""" + from simple_module_core.public_routes import PublicRouteRegistry + + registry = PublicRouteRegistry() + registry.add_exact("/api/branding/logo", methods=["GET"]) + app = _build_app(enabled=True, public_routes=registry) + resp = await _post(app, "/api/branding/logo") + assert resp.status_code == 503 + async def test_no_registry_still_gates(self) -> None: """A degraded/missing registry must fail closed, same as no provider.""" resp = await _get(_build_app(enabled=True, public_routes=None), "/api/branding/logo") From 2bea26cea492ef3097f020aa9f1da820ca46f649 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 15:38:46 +0200 Subject: [PATCH 10/17] test(e2e): point the browser suite at the moved admin URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/e2e/test_audit_log_ui.py | 6 +++--- tests/e2e/test_i18n_rendering.py | 10 +++++----- tests/e2e/test_settings_ui.py | 6 +++--- tests/e2e/test_shell_ui.py | 6 +++--- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/e2e/test_audit_log_ui.py b/tests/e2e/test_audit_log_ui.py index e027c3c9..82f7da48 100644 --- a/tests/e2e/test_audit_log_ui.py +++ b/tests/e2e/test_audit_log_ui.py @@ -1,6 +1,6 @@ """E2E smoke test for the Audit Log admin UI. -Drives a real browser to /audit_log, verifies the page renders with data +Drives a real browser to /admin/audit-log, verifies the page renders with data captured by the framework's audit listener, and confirms a freshly-created Setting produces an audit entry with a resolved (non-empty) entity_id — the regression test for the two-phase capture fix. @@ -31,7 +31,7 @@ def test_audit_log_renders_with_data(page: Page, e2e_username: str, e2e_password page.goto("/") _login(page, e2e_username, e2e_password) - page.goto("/audit_log") + page.goto("/admin/audit-log/") expect(page.get_by_role("heading", name="Audit Log")).to_be_visible() @@ -95,7 +95,7 @@ def test_audit_log_filter_by_entity_type(page: Page, e2e_username: str, e2e_pass page.goto("/") _login(page, e2e_username, e2e_password) - page.goto("/audit_log?entity_type=User&action=updated") + page.goto("/admin/audit-log/?entity_type=User&action=updated") expect(page.get_by_role("heading", name="Audit Log")).to_be_visible() diff --git a/tests/e2e/test_i18n_rendering.py b/tests/e2e/test_i18n_rendering.py index e12b78e9..867dcdc7 100644 --- a/tests/e2e/test_i18n_rendering.py +++ b/tests/e2e/test_i18n_rendering.py @@ -46,12 +46,12 @@ _PAGES = [ ("Dashboard", "/dashboard/"), ("Files", "/file-storage/"), - ("Users", "/users/admin"), - ("Feature Flags", "/feature_flags/"), - ("Branding", "/branding/"), + ("Users", "/admin/users/"), + ("Feature Flags", "/admin/feature-flags/"), + ("Branding", "/admin/branding/"), ("Background Tasks", "/admin/background-tasks/"), - ("Settings", "/settings/"), - ("Audit Log", "/audit_log/"), + ("Settings", "/admin/settings/"), + ("Audit Log", "/admin/audit-log/"), ] diff --git a/tests/e2e/test_settings_ui.py b/tests/e2e/test_settings_ui.py index a93e553b..ec506a1f 100644 --- a/tests/e2e/test_settings_ui.py +++ b/tests/e2e/test_settings_ui.py @@ -1,6 +1,6 @@ """E2E smoke test for the Settings modules admin UI. -Drives a real browser through the sidebar layout at ``/settings/modules``, +Drives a real browser through the sidebar layout at ``/admin/settings/``, toggles a module setting, and verifies the change hot-reloads into ``app.state`` without a server restart by exercising a downstream endpoint whose behaviour flips when the setting flips. @@ -22,7 +22,7 @@ def _login(page: Page, username: str, password: str) -> None: page.locator("#password").fill(password) page.get_by_role("button", name="Log in").click() # Wait for the session cookie to land before navigating away, or - # /settings/modules bounces us back to login and the sidebar never renders. + # /admin/settings/ bounces us back to login and the sidebar never renders. page.wait_for_url("**/dashboard/**", timeout=15_000) @@ -35,7 +35,7 @@ def test_toggle_host_multi_tenant_persists( page.goto("/") _login(page, e2e_username, e2e_password) - page.goto("/settings/modules") + page.goto("/admin/settings/") expect(page.get_by_text("Host", exact=False)).to_be_visible() # Click the Host entry in the sidebar. diff --git a/tests/e2e/test_shell_ui.py b/tests/e2e/test_shell_ui.py index 084b37f3..f3a81f92 100644 --- a/tests/e2e/test_shell_ui.py +++ b/tests/e2e/test_shell_ui.py @@ -29,7 +29,7 @@ def test_breadcrumb_names_the_section_on_sub_pages( page.goto("/") _login(page, e2e_username, e2e_password) - page.goto("/users/admin/add") + page.goto("/admin/users/add") crumb = page.get_by_role("navigation", name="breadcrumb") expect(crumb.get_by_role("link", name="Users")).to_be_visible() expect(crumb.get_by_text("Add people")).to_be_visible() @@ -46,7 +46,7 @@ def test_command_palette_opens_filters_and_navigates( expect(palette).to_be_visible() palette.fill("Audit") page.keyboard.press("Enter") - page.wait_for_url("**/audit_log**", timeout=10_000) + page.wait_for_url("**/admin/audit-log**", timeout=10_000) # Reopen and close with Escape — no navigation this time. page.keyboard.press("Control+k") @@ -78,5 +78,5 @@ def test_user_search_treats_like_metacharacters_literally( page.goto("/") _login(page, e2e_username, e2e_password) - page.goto("/users/admin?q=_") + page.goto("/admin/users/?q=_") expect(page.get_by_text("No users match these filters")).to_be_visible() From f18ada8065b76f92385f5b17d465f6401c2de430 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 15:50:18 +0200 Subject: [PATCH 11/17] fix(ui): keep every admin screen reachable from the command palette MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tests/test_module_settings_render.py | 6 +++--- packages/ui/src/layouts/SidebarLayout.tsx | 17 +++++++++++++++-- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/modules/settings/tests/test_module_settings_render.py b/modules/settings/tests/test_module_settings_render.py index 260bfa48..0dcbf3ca 100644 --- a/modules/settings/tests/test_module_settings_render.py +++ b/modules/settings/tests/test_module_settings_render.py @@ -61,7 +61,7 @@ async def test_client_side_visit_does_not_500( authenticated_client: httpx.AsyncClient, ) -> None: """The reported bug: reaching the page by clicking the sidebar link.""" - resp = await authenticated_client.get("/settings/", headers=_INERTIA) + resp = await authenticated_client.get("/admin/settings/", headers=_INERTIA) assert resp.status_code != _SERVER_ERROR assert resp.status_code == _OK @@ -71,7 +71,7 @@ async def test_the_path_survives_as_a_string( app_with_path_setting: FastAPI, authenticated_client: httpx.AsyncClient, ) -> None: - resp = await authenticated_client.get("/settings/", headers=_INERTIA) + resp = await authenticated_client.get("/admin/settings/", headers=_INERTIA) modules = resp.json()["props"]["modules"] demo = next(m for m in modules if m["package"] == "pathdemo") @@ -84,7 +84,7 @@ async def test_full_page_load_still_works( authenticated_client: httpx.AsyncClient, ) -> None: """The path that always worked must keep working.""" - resp = await authenticated_client.get("/settings/") + resp = await authenticated_client.get("/admin/settings/") assert resp.status_code == _OK assert resp.headers["content-type"].startswith("text/html") diff --git a/packages/ui/src/layouts/SidebarLayout.tsx b/packages/ui/src/layouts/SidebarLayout.tsx index ebf304ee..14469ace 100644 --- a/packages/ui/src/layouts/SidebarLayout.tsx +++ b/packages/ui/src/layouts/SidebarLayout.tsx @@ -7,7 +7,7 @@ import { TooltipTrigger, } from '@simple-module-py/ui/components/ui/tooltip'; import type React from 'react'; -import { useState } from 'react'; +import { useMemo, useState } from 'react'; import { AppTopbar, activeSection, findSection } from '../components/AppTopbar'; import { BrandingBanner } from '../components/BrandingBanner'; import { BrandingFooter } from '../components/BrandingFooter'; @@ -88,6 +88,19 @@ function SidebarShell({ children, menuKey, theme, headerSlot, footerNavSlot }: S const closeSidebar = () => setSidebarOpen(false); const menuItems = menus?.[menuKey] ?? NO_ITEMS; + // ⌘K reaches everything the viewer can open, not just the shell they are + // standing in. Both sidebars are already filtered by roles and permissions, + // so this widens reach without offering anything they cannot use — whereas + // indexing only `menuKey` made every admin screen unreachable from the app + // shell the moment they moved to their own sidebar. + const paletteItems = useMemo(() => { + const primary = menus?.sidebar ?? NO_ITEMS; + const admin = menus?.adminSidebar ?? NO_ITEMS; + const seen = new Set(); + return [...primary, ...admin].filter((item) => + seen.has(item.url) ? false : (seen.add(item.url), true), + ); + }, [menus?.sidebar, menus?.adminSidebar]); const declaredSection = usePageSection(currentUrl); // Same "which entry does this page belong to" resolution AppTopbar uses for // the breadcrumb, so the sidebar highlight and the breadcrumb never disagree. @@ -248,7 +261,7 @@ function SidebarShell({ children, menuKey, theme, headerSlot, footerNavSlot }: S {/* Main content */}
Date: Fri, 21 Aug 2026 16:03:31 +0200 Subject: [PATCH 12/17] test(maintenance): lock in the InertiaCache/Maintenance ordering end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../hosting/tests/test_maintenance_mode.py | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/framework/hosting/tests/test_maintenance_mode.py b/framework/hosting/tests/test_maintenance_mode.py index 19325562..2799cb32 100644 --- a/framework/hosting/tests/test_maintenance_mode.py +++ b/framework/hosting/tests/test_maintenance_mode.py @@ -4,6 +4,7 @@ import httpx import pytest +from simple_module_hosting._inertia_cache import InertiaCacheMiddleware from simple_module_hosting.maintenance import MaintenanceMiddleware from starlette.applications import Starlette from starlette.responses import PlainTextResponse @@ -34,6 +35,7 @@ def _build_app( user: _User | None = None, provider: _Provider | None = _Provider(), public_routes=None, + with_inertia_cache: bool = False, ) -> Starlette: async def ok(request): return PlainTextResponse("app reached") @@ -65,6 +67,12 @@ class _AuthState: app.state.public_routes = public_routes app.add_middleware(MaintenanceMiddleware) + # Real pipeline order: InertiaCache sits directly outside Maintenance, so + # its send-wrapper is what actually receives the 503's response messages. + # Added second so it wraps Maintenance and is itself wrapped by _SeedUser, + # matching install_middleware's (... -> InertiaCache -> Maintenance -> ...). + if with_inertia_cache: + app.add_middleware(InertiaCacheMiddleware) # Stands in for AuthMiddleware, which runs further out and is what puts the # resolved user on request.state. Added last so it executes first, exactly # as the real pipeline orders them. @@ -231,3 +239,32 @@ async def test_no_registry_still_gates(self) -> None: """A degraded/missing registry must fail closed, same as no provider.""" resp = await _get(_build_app(enabled=True, public_routes=None), "/api/branding/logo") assert resp.status_code == 503 + + +class TestInertiaCacheOrdering: + """Pins the exact regression `test_middleware_order.py` guards structurally: + with `InertiaCacheMiddleware` sitting directly outside `MaintenanceMiddleware` + (real pipeline order), the 503's own short-circuited response — not just a + response from `self.app` — must still pass through InertiaCache's + send-wrapper. Built here rather than asserted from order alone because a + correct middleware *order* does not by itself prove a short-circuiting + middleware's response reaches the wrapper outside it; the ASGI `send` + plumbing has to actually carry it, which is what this exercises end to end. + """ + + async def test_maintenance_503_gets_private_no_store_for_an_inertia_request(self) -> None: + app = _build_app(enabled=True, with_inertia_cache=True) + resp = await _get(app, "/protected", headers={"X-Inertia": "true"}) + assert resp.status_code == 503 + assert resp.headers["cache-control"] == "private, no-store" + assert "etag" not in resp.headers + assert "x-inertia" in resp.headers.get("vary", "").lower() + + async def test_maintenance_503_is_not_forced_private_for_a_plain_json_request(self) -> None: + """Only the Inertia representation needs the cache guard — a bare API + caller's 503 keeps whatever caching (none, here) it already had.""" + app = _build_app(enabled=True, with_inertia_cache=True) + resp = await _get(app, "/api/thing") + assert resp.status_code == 503 + assert "cache-control" not in resp.headers + assert "vary" not in resp.headers From de62bd5b77312818453bb26c2e5c52d923352c8c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 16:06:26 +0200 Subject: [PATCH 13/17] test(perf): measure the moved admin routes, not their redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/loadtest/locustfile.py | 2 +- tests/perf/test_asset_integrity.py | 4 ++-- tests/perf/test_nav_perf.py | 6 +++--- tests/perf/test_page_load.py | 2 +- tests/perf/test_perceived.py | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/loadtest/locustfile.py b/tests/loadtest/locustfile.py index cb7a8168..231a1da7 100644 --- a/tests/loadtest/locustfile.py +++ b/tests/loadtest/locustfile.py @@ -56,7 +56,7 @@ def users_list_api(self) -> None: def users_list_view(self) -> None: page = random.randint(1, 50) self.client.get( - f"/users/admin?page={page}&per_page=20", headers=_INERTIA, name="/users/admin" + f"/admin/users/?page={page}&per_page=20", headers=_INERTIA, name="/admin/users/" ) @task(8) diff --git a/tests/perf/test_asset_integrity.py b/tests/perf/test_asset_integrity.py index 99b3335f..7a737bb9 100644 --- a/tests/perf/test_asset_integrity.py +++ b/tests/perf/test_asset_integrity.py @@ -23,7 +23,7 @@ pytestmark = [pytest.mark.perf, pytest.mark.e2e] -ROUTES = ("/users/login", "/dashboard/", "/audit_log/", "/users/admin") +ROUTES = ("/users/login", "/dashboard/", "/admin/audit-log/", "/admin/users/") _SETTLE_MS = 1500 _CLIENT_ERROR = 400 # Chrome reports a module served as text/html this way; it is the signature of @@ -100,7 +100,7 @@ def test_lazy_page_chunks_resolve_under_the_static_prefix( ) # A route whose page component is a lazily-imported chunk. - page.goto(f"{base_url}/audit_log/", wait_until="load") + page.goto(f"{base_url}/admin/audit-log/", wait_until="load") page.wait_for_timeout(_SETTLE_MS) stray = [u for u in js_urls if u.startswith("/assets/")] diff --git a/tests/perf/test_nav_perf.py b/tests/perf/test_nav_perf.py index 017a910b..71dee527 100644 --- a/tests/perf/test_nav_perf.py +++ b/tests/perf/test_nav_perf.py @@ -29,8 +29,8 @@ # enforces that no menu URL redirects. ROUTES = ( ("dashboard", "/dashboard/"), - ("users_admin", "/users/admin"), - ("audit_log", "/audit_log/"), + ("users_admin", "/admin/users/"), + ("audit_log", "/admin/audit-log/"), ) @@ -111,7 +111,7 @@ def _on_response(response) -> None: page.on("response", _on_response) try: - measure_navigation(page, lambda: _click_sidebar(page, "/audit_log/"), "audit_log") + measure_navigation(page, lambda: _click_sidebar(page, "/admin/audit-log/"), "audit_log") finally: page.remove_listener("response", _on_response) diff --git a/tests/perf/test_page_load.py b/tests/perf/test_page_load.py index 8da53922..0eadad86 100644 --- a/tests/perf/test_page_load.py +++ b/tests/perf/test_page_load.py @@ -20,7 +20,7 @@ pytestmark = [pytest.mark.perf, pytest.mark.e2e] -ROUTES = ("/audit_log/", "/dashboard/") +ROUTES = ("/admin/audit-log/", "/dashboard/") # Compression must cut total transfer by at least this much. The observed # reduction is ~70%; 40% leaves generous headroom for bundle changes while # still failing loudly if compression silently stops being applied. diff --git a/tests/perf/test_perceived.py b/tests/perf/test_perceived.py index 07b6dd04..911136a3 100644 --- a/tests/perf/test_perceived.py +++ b/tests/perf/test_perceived.py @@ -33,8 +33,8 @@ ROUTES = ( ("dashboard", "/dashboard/"), - ("users_admin", "/users/admin"), - ("audit_log", "/audit_log/"), + ("users_admin", "/admin/users/"), + ("audit_log", "/admin/audit-log/"), ) _SETTLE_MS = 1200 # A 500px block inserted at the top of the body shifts essentially the whole From 2e35e515ef6c68926fad4282ac451ba8b788b37e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 16:25:35 +0200 Subject: [PATCH 14/17] fix: address code review findings (round 1, pass 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../simple_module_hosting/_error_handlers.py | 43 ++++++++-- .../simple_module_hosting/maintenance.py | 11 ++- .../simple_module_hosting/redirects.py | 22 +++--- .../tests/test_error_status_coverage.py | 79 +++++++++++++++++++ framework/hosting/tests/test_redirects.py | 29 +++++++ host/client_app/pages/Error.tsx | 9 ++- host/locales/es.json | 50 +++++++++--- modules/audit_log/audit_log/pages/Browse.tsx | 6 +- modules/dashboard/dashboard/pages/Home.tsx | 40 +++++++--- modules/dashboard/dashboard/stats.py | 4 + modules/dashboard/tests/test_dashboard.py | 22 ++++++ .../feature_flags/pages/Browse.tsx | 8 +- .../users/tests/test_users_login_deep_link.py | 54 +++++++++++++ .../users/users/admin/components/RolesTab.tsx | 2 +- modules/users/users/admin/views.py | 4 +- modules/users/users/oauth/api.py | 14 +++- .../pages/Users/components/RolesCard.tsx | 2 +- packages/i18n/src/generated-resources.ts | 1 + packages/i18n/src/keys.generated.ts | 3 + packages/ui/locales/en.json | 3 + packages/ui/locales/es.json | 3 + .../ui/src/layouts/AdminSectionLink.test.tsx | 37 +++++++++ packages/ui/src/layouts/AdminSectionLink.tsx | 49 ++++++++++++ packages/ui/src/layouts/SidebarLayout.tsx | 30 +++++-- tests/test_audit_log.py | 2 +- tests/test_principal_resolver_integration.py | 4 +- 26 files changed, 463 insertions(+), 68 deletions(-) create mode 100644 packages/ui/src/layouts/AdminSectionLink.test.tsx create mode 100644 packages/ui/src/layouts/AdminSectionLink.tsx diff --git a/framework/hosting/simple_module_hosting/_error_handlers.py b/framework/hosting/simple_module_hosting/_error_handlers.py index 2b09d87e..87e3a7e9 100644 --- a/framework/hosting/simple_module_hosting/_error_handlers.py +++ b/framework/hosting/simple_module_hosting/_error_handlers.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +from collections.abc import Mapping from fastapi.encoders import jsonable_encoder from fastapi.exceptions import RequestValidationError @@ -108,7 +109,20 @@ def _wants_json(request: Request) -> bool: return is_api or bool(json_q) -async def render_error_page(request: Request, status_code: int, message: str) -> Response: +async def render_error_page( + request: Request, + status_code: int, + message: str, + headers: Mapping[str, str] | None = None, +) -> Response: + """Render the Inertia error page for *status_code*. + + ``headers`` carries an ``HTTPException``'s own headers through to the + response. A rendered page is still the same status as the JSON body it + replaces, so ``WWW-Authenticate`` on a 401 and ``Retry-After`` on a + 429/503 have to survive the switch — widening the set of statuses that + render a page must not quietly narrow what those responses carry. + """ try: # Inside the try, not above it: this lookup is exactly the kind of # thing that is missing when the app is half-built, and an error page @@ -139,6 +153,7 @@ async def render_error_page(request: Request, status_code: int, message: str) -> }, ) response.status_code = status_code + _apply_headers(response, headers) return response except InertiaVersionConflictException as exc: return await inertia_version_conflict_exception_handler(request, exc) @@ -146,20 +161,36 @@ async def render_error_page(request: Request, status_code: int, message: str) -> # Fallback if Inertia rendering itself fails (e.g. missing session) logger.exception("Error page rendering failed, falling back to JSON") return JSONResponse( - status_code=status_code, content={"detail": message or "Internal Server Error"} + status_code=status_code, + content={"detail": message or "Internal Server Error"}, + headers=dict(headers) if headers else None, ) +def _apply_headers(response: Response, headers: Mapping[str, str] | None) -> None: + """Copy exception headers onto an already-built response. + + Set rather than appended: these are single-valued response headers, and + a duplicate ``Retry-After`` is worse than none. + """ + if not headers: + return + for key, value in headers.items(): + response.headers[key] = value + + async def http_exception_handler(request: Request, exc: HTTPException) -> Response: + # Preserve exception headers (WWW-Authenticate, Retry-After, ...) the way + # FastAPI's stock handler does — on the rendered page as well as the JSON + # body, since both answer with the same status. + headers = getattr(exc, "headers", None) if exc.status_code in _INERTIA_ERROR_STATUSES and not _wants_json(request): detail = str(exc.detail) if exc.detail else "" - return await render_error_page(request, exc.status_code, detail) - # Preserve exception headers (WWW-Authenticate, Retry-After, ...) the way - # FastAPI's stock handler does. + return await render_error_page(request, exc.status_code, detail, headers) return JSONResponse( status_code=exc.status_code, content={"detail": exc.detail}, - headers=getattr(exc, "headers", None), + headers=headers, ) diff --git a/framework/hosting/simple_module_hosting/maintenance.py b/framework/hosting/simple_module_hosting/maintenance.py index 58a3fc89..c4e8925c 100644 --- a/framework/hosting/simple_module_hosting/maintenance.py +++ b/framework/hosting/simple_module_hosting/maintenance.py @@ -30,6 +30,11 @@ "/i18n/", ) +# Both representations of the 503 advertise the same retry window, so it is +# stated once. An hour is a guess by construction — the switch carries no +# end time — but a client that backs off for an hour beats one that hammers. +_RETRY_AFTER = {"Retry-After": "3600"} + __all__ = ["MaintenanceMiddleware"] @@ -115,8 +120,6 @@ async def _render(request: Request, message: str): return JSONResponse( status_code=503, content={"detail": message or "Service temporarily unavailable"}, - headers={"Retry-After": "3600"}, + headers=_RETRY_AFTER, ) - response = await render_error_page(request, 503, message) - response.headers["Retry-After"] = "3600" - return response + return await render_error_page(request, 503, message, _RETRY_AFTER) diff --git a/framework/hosting/simple_module_hosting/redirects.py b/framework/hosting/simple_module_hosting/redirects.py index f813a0a9..973d8676 100644 --- a/framework/hosting/simple_module_hosting/redirects.py +++ b/framework/hosting/simple_module_hosting/redirects.py @@ -11,28 +11,30 @@ from urllib.parse import urlsplit from fastapi import Request +from simple_module_core.redirect_safety import safe_next def safe_referer_or_root(request: Request) -> str: """Return the Referer iff it's same-origin; otherwise fall back to ``/``. Only honors references that (a) resolve to the same scheme+host as the - current request, or (b) are relative paths that don't try to escape to a - protocol-relative URL (``//evil.example``). + current request, or (b) are relative paths. Either way the result is run + through :func:`~simple_module_core.redirect_safety.safe_next`, which is the + single owner of what counts as a safe same-site target. """ referer = request.headers.get("referer") if not referer: return "/" - # Protocol-relative URLs like "//evil.example/foo" resolve against the - # origin in browsers but leave the site — reject them. - if referer.startswith("//"): - return "/" - parsed = urlsplit(referer) - # Relative path with no scheme+host → same-origin by construction. + # Relative reference (no scheme+host). ``safe_next`` owns the rules here — + # it rejects protocol-relative ("//host") and backslash-prefixed ("/\\host") + # targets, both of which browsers resolve off-site, plus anything carrying + # CR/LF that could be smuggled into the Location header. Delegated rather + # than restated so the two sanitisers cannot drift: a second copy that + # missed one of those cases is exactly how this becomes an open redirect. if not parsed.scheme and not parsed.netloc: - return referer if referer.startswith("/") else "/" + return safe_next(referer) # Absolute URL → must match the current request's origin. current = request.url @@ -40,6 +42,6 @@ def safe_referer_or_root(request: Request) -> str: path = parsed.path or "/" if parsed.query: path = f"{path}?{parsed.query}" - return path + return safe_next(path) return "/" diff --git a/framework/hosting/tests/test_error_status_coverage.py b/framework/hosting/tests/test_error_status_coverage.py index 5f852148..849d0406 100644 --- a/framework/hosting/tests/test_error_status_coverage.py +++ b/framework/hosting/tests/test_error_status_coverage.py @@ -148,3 +148,82 @@ async def test_half_built_app_falls_back_to_json(self) -> None: assert resp.status_code == 500 assert b"kaboom" in resp.body + + +class TestExceptionHeadersSurvive: + """An ``HTTPException``'s headers must reach the caller on the rendered page + too, not only on the JSON body. + + Widening ``_INERTIA_ERROR_STATUSES`` to cover 401/429/503 moved exactly the + statuses whose headers carry meaning — ``WWW-Authenticate``, ``Retry-After`` + — onto the page-rendering branch. If that branch drops them, the framework + quietly stops honouring a contract its own comment still claims. + """ + + @staticmethod + def _request(app, headers: list[tuple[bytes, bytes]] | None = None): + from starlette.requests import Request + + return Request( + { + "type": "http", + "http_version": "1.1", + "method": "GET", + "path": "/gated", + "raw_path": b"/gated", + "query_string": b"", + "scheme": "http", + "server": ("testserver", 80), + "client": ("testclient", 1234), + "headers": headers or [], + "app": app, + } + ) + + async def test_render_fallback_carries_the_headers(self) -> None: + """Even the half-built-app JSON fallback keeps them.""" + from simple_module_hosting._error_handlers import render_error_page + from starlette.applications import Starlette + + resp = await render_error_page( + self._request(Starlette()), + 429, + "slow down", + {"Retry-After": "60"}, + ) + + assert resp.status_code == 429 + assert resp.headers["Retry-After"] == "60" + + async def test_http_exception_handler_forwards_them_to_the_page(self) -> None: + from simple_module_hosting._error_handlers import http_exception_handler + from starlette.applications import Starlette + from starlette.exceptions import HTTPException + + # Accept: text/html makes this browser-shaped, so it takes the + # page-rendering branch rather than the JSON one. + request = self._request(Starlette(), [(b"accept", b"text/html")]) + exc = HTTPException( + status_code=401, + detail="Not authenticated", + headers={"WWW-Authenticate": "Bearer"}, + ) + + resp = await http_exception_handler(request, exc) + + assert resp.status_code == 401 + assert resp.headers["WWW-Authenticate"] == "Bearer" + + async def test_json_branch_still_carries_them(self) -> None: + """The pre-existing behaviour this must not regress.""" + from simple_module_hosting._error_handlers import http_exception_handler + from starlette.applications import Starlette + from starlette.exceptions import HTTPException + + request = self._request(Starlette(), [(b"accept", b"application/json")]) + exc = HTTPException(429, "Too many", headers={"Retry-After": "30"}) + + resp = await http_exception_handler(request, exc) + + assert resp.status_code == 429 + assert resp.headers["Retry-After"] == "30" diff --git a/framework/hosting/tests/test_redirects.py b/framework/hosting/tests/test_redirects.py index e7090cb8..4e6d125c 100644 --- a/framework/hosting/tests/test_redirects.py +++ b/framework/hosting/tests/test_redirects.py @@ -61,6 +61,8 @@ class TestSafeRefererBlocksHostedirects: "//evil.example/x", "//evil.example", r"\\evil.example/x", # backslash-prefixed — some browsers normalize + "/\\evil.example", # relative-looking, but browsers resolve it off-site + "/\\\\evil.example/x", "http://testserver.evil.example/", # suffix-confusion "http://evil.example@testserver/", # userinfo trick: host is "evil.example" "javascript:alert(1)", @@ -127,3 +129,30 @@ def test_fragment_dropped(self) -> None: def test_empty_path_becomes_root(self) -> None: req = _make_request(referer="http://testserver") assert safe_referer_or_root(req) == "/" + + +class TestDelegatesToSafeNext: + """``safe_referer_or_root`` funnels its result through ``safe_next``. + + Two sanitisers for one job is how one of them ends up missing a case: the + inline rules here accepted ``/\\evil.example`` and CR/LF-carrying paths + that ``safe_next`` rejects, so the protection you got depended on which + helper the call site happened to import. + """ + + def test_backslash_prefixed_relative_target_is_rejected(self) -> None: + """``urlsplit`` reports no scheme and no netloc for this, so the + relative branch used to hand it straight back — and browsers resolve + it against ``evil.example``.""" + req = _make_request(referer="/\\evil.example") + assert safe_referer_or_root(req) == "/" + + def test_crlf_in_a_relative_target_is_rejected(self) -> None: + """Otherwise smuggled into the Location header.""" + req = _make_request(referer="/ok\r\nLocation: https://evil.example") + assert safe_referer_or_root(req) == "/" + + def test_ordinary_same_site_path_still_passes(self) -> None: + assert safe_referer_or_root(_make_request(referer="/admin/users/?page=2")) == ( + "/admin/users/?page=2" + ) diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index 0821ab3d..8149f8f8 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -92,9 +92,6 @@ function useStatusCopy(status: number, maintenance: boolean): StatusCopy { ); } -/** Statuses where "sign in" is the remedy, not "go home". */ -const SIGN_IN_STATUSES = new Set([401, 419]); - function ErrorPage({ status, message, correlation_id, login_url, maintenance }: Props) { const { t } = useT(); const copy = useStatusCopy(status, Boolean(maintenance)); @@ -102,7 +99,11 @@ function ErrorPage({ status, message, correlation_id, login_url, maintenance }: // A server-supplied message wins over the canned description — it is the // specific reason, where the table only knows the status class. const description = message || copy.description; - const showSignIn = SIGN_IN_STATUSES.has(status) && Boolean(login_url); + // The server already decided this: `login_url` is sent only for the + // statuses in `_SIGN_IN_STATUSES` and is null otherwise. Re-deriving the + // list here would mean editing it in two languages, where missing one + // silently hides the button rather than failing. + const showSignIn = Boolean(login_url); return ( <> diff --git a/host/locales/es.json b/host/locales/es.json index 96800632..b926d084 100644 --- a/host/locales/es.json +++ b/host/locales/es.json @@ -1,4 +1,37 @@ { + "admin": { + "title": "Administración", + "description": "Gestiona usuarios, accesos, apariencia y configuración del sistema.", + "empty": "No hay herramientas de administración disponibles para tu cuenta.", + "overview": "Resumen" + }, + "error": { + "correlation_id_copy": "Copiar ID de referencia", + "correlation_id_label": "Menciona este ID si contactas con soporte", + "forbidden_description": "No tienes permiso para acceder a esta página.", + "forbidden_title": "Prohibido", + "generic_description": "Ocurrió un error inesperado.", + "generic_title": "Error", + "go_back": "Volver", + "go_home": "Ir al inicio", + "invalid_request_description": "La dirección que seguiste contiene parámetros que no pudimos leer.", + "invalid_request_title": "Solicitud no válida", + "maintenance_description": "Estamos haciendo algunos cambios y volveremos en breve. Gracias por tu paciencia.", + "maintenance_title": "En mantenimiento", + "not_found_description": "La página que buscas no existe o ha sido movida.", + "not_found_title": "Página no encontrada", + "rate_limited_description": "Has hecho muchas solicitudes en poco tiempo. Espera un momento e inténtalo de nuevo.", + "rate_limited_title": "Demasiadas solicitudes", + "server_error_description": "Algo salió mal de nuestro lado. Por favor, inténtalo de nuevo más tarde.", + "server_error_title": "Error del servidor", + "session_expired_description": "Se cerró tu sesión tras un periodo de inactividad. Inicia sesión de nuevo para continuar donde lo dejaste.", + "session_expired_title": "Sesión caducada", + "sign_in": "Iniciar sesión", + "unauthorized_description": "Debes iniciar sesión para ver esta página.", + "unauthorized_title": "Inicio de sesión requerido", + "unavailable_description": "El servicio no está disponible temporalmente. Inténtalo de nuevo en unos momentos.", + "unavailable_title": "Servicio no disponible" + }, "landing": { "badge": "v0.1 · Python 3.12 · experimental", "hero_title_line1": "Monolitos modulares para Python —", @@ -22,18 +55,9 @@ "devtools_description": "SQLAlchemy async + Pydantic + Alembic. Genera migraciones por módulo." } }, - "error": { - "generic_title": "Error", - "generic_description": "Ocurrió un error inesperado.", - "forbidden_title": "Prohibido", - "forbidden_description": "No tienes permiso para acceder a esta página.", - "not_found_title": "Página no encontrada", - "not_found_description": "La página que buscas no existe o ha sido movida.", - "server_error_title": "Error del servidor", - "server_error_description": "Algo salió mal de nuestro lado. Por favor, inténtalo de nuevo más tarde.", - "go_home": "Ir al inicio", - "go_back": "Volver", - "correlation_id_label": "Menciona este ID si contactas con soporte", - "correlation_id_copy": "Copiar ID de referencia" + "offline": { + "title": "Sin conexión", + "description": "Es posible que los cambios no se guarden hasta que se restablezca la conexión.", + "restored": "Conexión restablecida" } } diff --git a/modules/audit_log/audit_log/pages/Browse.tsx b/modules/audit_log/audit_log/pages/Browse.tsx index 90b84f41..19a188cf 100644 --- a/modules/audit_log/audit_log/pages/Browse.tsx +++ b/modules/audit_log/audit_log/pages/Browse.tsx @@ -134,7 +134,11 @@ function Browse() { if (next.toDate) p.to_date = next.toDate; if (nextPage > 1) p.page = String(nextPage); if (page_size !== 50) p.page_size = String(page_size); - router.visit(`/admin/audit-log?${new URLSearchParams(p).toString()}`); + // Trailing slash: the browse route is registered at "/" under + // VIEW_PREFIX and reaches the app via `include_router`, which + // `_clone_bare_prefix_route` cannot alias — the bare form costs a 307 + // on every filter change. Matches MENU_URL in constants.py. + router.visit(`/admin/audit-log/?${new URLSearchParams(p).toString()}`); } function handleClear() { diff --git a/modules/dashboard/dashboard/pages/Home.tsx b/modules/dashboard/dashboard/pages/Home.tsx index d8c44c8a..855eb02d 100644 --- a/modules/dashboard/dashboard/pages/Home.tsx +++ b/modules/dashboard/dashboard/pages/Home.tsx @@ -15,6 +15,9 @@ interface SystemModule { status: 'loaded'; /** The module's own screen, or '' when it ships no views. */ url: string; + /** Second mount point for a module that is only partly administrative + * (`ModuleMeta.admin_view_prefix`), or '' when it declares none. */ + admin_url: string; /** Worst health status across the module's checks; '' when it registers none. */ health: '' | 'healthy' | 'degraded' | 'unhealthy'; } @@ -68,7 +71,8 @@ function Home() { // Every module's own prefix, so the fallback below can tell "this entry is // mine" from "this entry belongs to a module mounted deeper than me". const modulePrefixes = props.system_info.modules - .map((m) => m.url.replace(/\/+$/, '')) + .flatMap((m) => [m.url, m.admin_url]) + .map((url) => url.replace(/\/+$/, '')) .filter(Boolean); /** @@ -76,17 +80,31 @@ function Home() { * none. * * Matching the view prefix exactly is not enough: a module often mounts its - * landing screen below its own prefix (Users is `/users`, its menu entry is - * `/users/admin`), and an exact match leaves those tiles permanently inert - * for admins who can in fact open them. So fall back to the first menu entry - * that lives under the prefix — but only if no *other* module owns a longer - * prefix of that entry, or a module mounted at `/admin` would adopt the - * background-tasks entry at `/admin/background-tasks` and link its tile to - * somebody else's screen. + * landing screen below its own prefix (background_tasks is + * `/admin/background-tasks`, its menu entry is `/admin/background-tasks/`), + * and an exact match leaves those tiles permanently inert for admins who can + * in fact open them. So fall back to the first menu entry that lives under + * the prefix — but only if no *other* module owns a longer prefix of that + * entry, or a module mounted at `/admin` would adopt the background-tasks + * entry and link its tile to somebody else's screen. + * + * A module that is only partly administrative mounts its admin screens + * outside its own `view_prefix` (Users serves sign-in at `/users` and + * management at `/admin/users`), so both prefixes are searched — admin + * first, since that is the screen the tile is for. Without it the Users + * tile falls through to `/users/me` and opens the viewer's own profile. */ - function menuTarget(url: string): string { - if (!url) return ''; + function menuTarget(url: string, adminUrl: string): string { + for (const candidate of [adminUrl, url]) { + const hit = candidate ? resolveUnderPrefix(candidate) : ''; + if (hit) return hit; + } + return ''; + } + + function resolveUnderPrefix(url: string): string { const prefix = url.replace(/\/+$/, ''); + if (!prefix) return ''; const exact = menuUrls.find((menuUrl) => menuUrl.replace(/\/+$/, '') === prefix); if (exact) return exact; return ( @@ -152,7 +170,7 @@ function Home() {
{props.system_info.modules.map((m) => { - const target = menuTarget(m.url); + const target = menuTarget(m.url, m.admin_url); return ( list[ "name": m.meta.name, "status": "loaded", "url": f"{m.meta.view_prefix}/" if m.meta.view_prefix else "", + # A partly-administrative module (users) mounts its management + # screens outside its own view_prefix, so the tile cannot find + # them by prefix alone — ship the second mount point too. + "admin_url": (f"{m.meta.admin_view_prefix}/" if m.meta.admin_view_prefix else ""), "health": worst.get(m.meta.name, ""), } for m in app.state.sm.modules diff --git a/modules/dashboard/tests/test_dashboard.py b/modules/dashboard/tests/test_dashboard.py index 21c5d63d..34cf21c7 100644 --- a/modules/dashboard/tests/test_dashboard.py +++ b/modules/dashboard/tests/test_dashboard.py @@ -107,6 +107,28 @@ async def test_module_entries_carry_a_link_target( modules = {m["name"]: m for m in resp.json()["system_info"]["modules"]} assert modules["Dashboard"]["url"] == "/dashboard/" + async def test_partly_admin_modules_carry_their_admin_mount( + self, authenticated_client: httpx.AsyncClient + ): + """Users serves sign-in at /users and management at /admin/users. + + The tile's job is to open the management screen, which no longer lives + under the module's own ``view_prefix`` — without the second mount point + in the payload the tile falls through to the first menu entry under + ``/users`` (the viewer's own profile) and opens the wrong page. + """ + resp = await authenticated_client.get(_STATS_URL) + modules = {m["name"]: m for m in resp.json()["system_info"]["modules"]} + assert modules["Users"]["url"] == "/users/" + assert modules["Users"]["admin_url"] == "/admin/users/" + + async def test_modules_without_an_admin_mount_report_an_empty_admin_url( + self, authenticated_client: httpx.AsyncClient + ): + resp = await authenticated_client.get(_STATS_URL) + for mod in resp.json()["system_info"]["modules"]: + assert mod["admin_url"] == "" or mod["admin_url"].startswith("/"), mod + async def test_view_less_modules_get_an_empty_url( self, authenticated_client: httpx.AsyncClient ): diff --git a/modules/feature_flags/feature_flags/pages/Browse.tsx b/modules/feature_flags/feature_flags/pages/Browse.tsx index 81b1307a..6f5633be 100644 --- a/modules/feature_flags/feature_flags/pages/Browse.tsx +++ b/modules/feature_flags/feature_flags/pages/Browse.tsx @@ -43,9 +43,13 @@ interface Props { } function buildPath(tenantId: string | null) { + // Trailing slash: the browse route is registered at "/" under the module's + // view prefix, and `_clone_bare_prefix_route` cannot alias a bare prefix for + // routes contributed via `include_router` — so the bare form costs a 307 on + // every navigation. Matches MENU_URL in constants.py. return tenantId - ? `/admin/feature-flags?tenant_id=${encodeURIComponent(tenantId)}` - : '/admin/feature-flags'; + ? `/admin/feature-flags/?tenant_id=${encodeURIComponent(tenantId)}` + : '/admin/feature-flags/'; } function actionUrl(name: string, action: 'toggle' | 'clear', tenantId: string | null) { diff --git a/modules/users/tests/test_users_login_deep_link.py b/modules/users/tests/test_users_login_deep_link.py index 1a8f5f49..914efa22 100644 --- a/modules/users/tests/test_users_login_deep_link.py +++ b/modules/users/tests/test_users_login_deep_link.py @@ -61,3 +61,57 @@ async def test_without_a_bounce_the_default_is_used(self, anon_client): headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) assert resp.json()["props"]["login_redirect_url"] == "/dashboard/" + + +class TestEveryProviderConsumesTheStashedTarget: + """All three completion paths must honour ``SESSION_NEXT_KEY`` and clear it. + + ``AuthMiddleware`` writes the key for every bounced request, whichever + provider the visitor eventually picks. A path that never reads it silently + drops the deep link; a path that reads without popping leaves a stale + target to fire on some later, unrelated visit to the login page. The OAuth + callback did neither, so signing in with Google lost the deep link that the + password and Keycloak paths kept — the exact per-module drift the shared + key exists to prevent. + + Asserted against the source because these are three different flows in + three packages, two of which need a live identity provider to exercise + end to end; the thing worth pinning is that none of them forgets. + """ + + @staticmethod + def _source(relative: str) -> str: + from pathlib import Path + + root = Path(__file__).resolve().parents[3] + return (root / relative).read_text(encoding="utf-8") + + @pytest.mark.parametrize( + "relative", + [ + "modules/users/users/oauth/api.py", + "modules/users/users/auth_local/api.py", + "modules/keycloak/keycloak/endpoints/api.py", + ], + ) + def test_login_completion_paths_clear_the_stashed_target(self, relative: str) -> None: + source = self._source(relative) + assert "SESSION_NEXT_KEY" in source, ( + f"{relative} completes a login without touching the shared post-login destination key" + ) + assert "pop(SESSION_NEXT_KEY" in source, ( + f"{relative} must pop SESSION_NEXT_KEY once login succeeds, or a " + "stale deep link fires on a later visit to the login page" + ) + + @pytest.mark.parametrize( + "relative", + [ + "modules/users/users/oauth/api.py", + "modules/keycloak/keycloak/endpoints/api.py", + ], + ) + def test_redirecting_providers_sanitise_the_target(self, relative: str) -> None: + """The value lands in a Location header, so it is re-checked on the way + out even though AuthMiddleware already validated it going in.""" + assert "safe_next_or_none" in self._source(relative) diff --git a/modules/users/users/admin/components/RolesTab.tsx b/modules/users/users/admin/components/RolesTab.tsx index 6c4f30dc..95970a3c 100644 --- a/modules/users/users/admin/components/RolesTab.tsx +++ b/modules/users/users/admin/components/RolesTab.tsx @@ -61,7 +61,7 @@ export function RolesTab({ roles }: { roles: RoleItem[] }) {
diff --git a/modules/users/users/admin/views.py b/modules/users/users/admin/views.py index e0e9dc62..cdea9141 100644 --- a/modules/users/users/admin/views.py +++ b/modules/users/users/admin/views.py @@ -131,7 +131,7 @@ async def admin_add_people_page( dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) async def admin_invite_redirect() -> RedirectResponse: - """Old invite URL — the flow merged into /users/admin/add.""" + """Old invite URL — the flow merged into /admin/users/add.""" return RedirectResponse("/admin/users/add?mode=invite", status_code=307) @@ -141,7 +141,7 @@ async def admin_invite_redirect() -> RedirectResponse: dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) async def admin_create_redirect() -> RedirectResponse: - """Old create URL — the flow merged into /users/admin/add.""" + """Old create URL — the flow merged into /admin/users/add.""" return RedirectResponse("/admin/users/add?mode=create", status_code=307) diff --git a/modules/users/users/oauth/api.py b/modules/users/users/oauth/api.py index a86cf3b5..cc475c98 100644 --- a/modules/users/users/oauth/api.py +++ b/modules/users/users/oauth/api.py @@ -9,7 +9,8 @@ Why a custom handler rather than ``fastapi_users.get_oauth_router``: the stock ``/callback`` returns 204; Inertia needs the browser to land on a real page, so -``/callback`` returns a 303 redirect to ``login_redirect_url`` with the auth +``/callback`` returns a 303 redirect to the stashed deep link (or +``login_redirect_url``) with the auth cookie attached. Find-or-create + email-association go through ``UserManager.oauth_callback``. State CSRF uses Starlette's signed session cookie. @@ -22,6 +23,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request, status from fastapi_users import exceptions as fu_exceptions +from simple_module_core.redirect_safety import SESSION_NEXT_KEY, safe_next_or_none from starlette.responses import RedirectResponse from users.constants import OAUTH_REGISTRATION_REQUEST_FLAG @@ -117,7 +119,15 @@ async def callback( login_response = await auth_backend.login(strategy, user) await user_manager.on_after_login(user, request, login_response) - redirect_url = request.app.state.users.settings.login_redirect_url + # Honour the deep link AuthMiddleware stashed before bouncing this + # visitor to login, exactly as the password and Keycloak paths do. + # Popped, not read: leaving it would send some later, unrelated visit + # to /users/login off to a stale destination. Re-sanitised on the way + # out because the value lands in a Location header. + redirect_url = ( + safe_next_or_none(request.session.pop(SESSION_NEXT_KEY, None)) + or request.app.state.users.settings.login_redirect_url + ) redirect = RedirectResponse(redirect_url, status_code=303) for key, value in login_response.headers.items(): if key.lower() == "set-cookie": diff --git a/modules/users/users/pages/Users/components/RolesCard.tsx b/modules/users/users/pages/Users/components/RolesCard.tsx index 233a7bb0..9f3b319c 100644 --- a/modules/users/users/pages/Users/components/RolesCard.tsx +++ b/modules/users/users/pages/Users/components/RolesCard.tsx @@ -24,7 +24,7 @@ export function RolesCard({ roles, selected, onToggle, userId, hasPermissionsMod {hasPermissionsModule && ( diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 229e8747..5ddae711 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -401,6 +401,7 @@ export default { 'ui.errors.generic_title': '', 'ui.errors.go_home_button': '', 'ui.errors.reload_button': '', + 'ui.nav.admin': '', 'ui.switcher.label': '', 'users.empty.add_people': '', 'users.empty.clear_filters': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index f7e27757..6ae64617 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -536,6 +536,9 @@ export const keys = { go_home_button: 'ui.errors.go_home_button', reload_button: 'ui.errors.reload_button', }, + nav: { + admin: 'ui.nav.admin', + }, switcher: { label: 'ui.switcher.label', }, diff --git a/packages/ui/locales/en.json b/packages/ui/locales/en.json index 89d43d44..44e2de34 100644 --- a/packages/ui/locales/en.json +++ b/packages/ui/locales/en.json @@ -5,6 +5,9 @@ "reload_button": "Reload Page", "go_home_button": "Go Home" }, + "nav": { + "admin": "Administration" + }, "switcher": { "label": "Change language" } diff --git a/packages/ui/locales/es.json b/packages/ui/locales/es.json index ab676973..7b65c825 100644 --- a/packages/ui/locales/es.json +++ b/packages/ui/locales/es.json @@ -5,6 +5,9 @@ "reload_button": "Recargar página", "go_home_button": "Ir al inicio" }, + "nav": { + "admin": "Administración" + }, "switcher": { "label": "Cambiar idioma" } diff --git a/packages/ui/src/layouts/AdminSectionLink.test.tsx b/packages/ui/src/layouts/AdminSectionLink.test.tsx new file mode 100644 index 00000000..5e5bf3b5 --- /dev/null +++ b/packages/ui/src/layouts/AdminSectionLink.test.tsx @@ -0,0 +1,37 @@ +import { render, screen } from '@testing-library/react'; +import { describe, expect, test, vi } from 'vitest'; +import type { MenuItem } from '../types'; + +// Mock @simple-module-py/i18n so useT resolves the nav.admin key without a +// real i18next instance — same pattern LocaleSwitcher.test.tsx uses. +vi.mock('@simple-module-py/i18n', () => ({ + useT: () => ({ + t: (key: string) => (key === 'ui.nav.admin' ? 'Administration' : key), + }), + keys: { + ui: { + nav: { + admin: 'ui.nav.admin', + }, + }, + }, +})); + +import { AdminSectionLink } from './AdminSectionLink'; + +const adminItem: MenuItem = { label: 'Users', url: '/admin/users/', icon: 'users' }; + +describe('AdminSectionLink', () => { + test('renders a link to /admin when the viewer has admin entries', () => { + render( {}} />); + const link = screen.getByRole('link', { name: 'Administration' }); + expect(link).toHaveAttribute('href', '/admin'); + }); + + test('renders nothing when the viewer has no admin entries', () => { + const { container } = render( + {}} />, + ); + expect(container).toBeEmptyDOMElement(); + }); +}); diff --git a/packages/ui/src/layouts/AdminSectionLink.tsx b/packages/ui/src/layouts/AdminSectionLink.tsx new file mode 100644 index 00000000..8d207c14 --- /dev/null +++ b/packages/ui/src/layouts/AdminSectionLink.tsx @@ -0,0 +1,49 @@ +import { Link } from '@inertiajs/react'; +import { keys, useT } from '@simple-module-py/i18n'; +import type React from 'react'; +import { NavIcon } from '../components/NavIcon'; +import type { MenuItem } from '../types'; + +interface AdminSectionLinkProps { + /** The viewer's admin menu, already filtered by roles and permissions. */ + adminItems: MenuItem[]; + /** Sidebar `inactiveClass` from the host layout's theme. */ + className: string; + onNavigate: () => void; +} + +/** + * The app shell's doorway into the admin section — the counterpart of + * `AdminLayout`'s "Back to App". + * + * Every admin screen moved out of the main sidebar into `adminSidebar`, so + * without this an admin signs in, lands on `/dashboard/`, and has no link to + * Users, Settings, Branding or anything else they administer: the entries they + * used before are simply gone from the shell. + * + * Renders nothing when the viewer has no admin entries. `adminSidebar` is + * already filtered by roles *and* permissions, so a non-empty list is exactly + * "this account has somewhere to go" — and `/admin` admits on that same + * signal, so this never offers a link that would 403. + */ +export function AdminSectionLink({ + adminItems, + className, + onNavigate, +}: AdminSectionLinkProps): React.ReactElement | null { + const { t } = useT(); + if (adminItems.length === 0) return null; + + return ( +
+ + + {t(keys.ui.nav.admin)} + +
+ ); +} diff --git a/packages/ui/src/layouts/SidebarLayout.tsx b/packages/ui/src/layouts/SidebarLayout.tsx index 14469ace..b35715ee 100644 --- a/packages/ui/src/layouts/SidebarLayout.tsx +++ b/packages/ui/src/layouts/SidebarLayout.tsx @@ -18,6 +18,7 @@ import { NavIcon } from '../components/NavIcon'; import { PageHeadingProvider, usePageSection } from '../components/page-heading'; import { darkSurfaceLogo } from '../lib/brand'; import type { MenuItem, SharedProps } from '../types'; +import { AdminSectionLink } from './AdminSectionLink'; import { SidebarUserMenu } from './SidebarUserMenu'; // A stable reference for "no items" — `menus?.[key] ?? []` would otherwise @@ -93,14 +94,20 @@ function SidebarShell({ children, menuKey, theme, headerSlot, footerNavSlot }: S // so this widens reach without offering anything they cannot use — whereas // indexing only `menuKey` made every admin screen unreachable from the app // shell the moment they moved to their own sidebar. - const paletteItems = useMemo(() => { - const primary = menus?.sidebar ?? NO_ITEMS; - const admin = menus?.adminSidebar ?? NO_ITEMS; - const seen = new Set(); - return [...primary, ...admin].filter((item) => - seen.has(item.url) ? false : (seen.add(item.url), true), - ); - }, [menus?.sidebar, menus?.adminSidebar]); + // Keyed by url so a module that somehow contributes the same destination to + // both sections is listed once. MenuRegistry buckets each item into exactly + // one section, so this is a cheap invariant guard rather than a live case. + const paletteItems = useMemo( + () => + Array.from( + new Map( + [...(menus?.sidebar ?? NO_ITEMS), ...(menus?.adminSidebar ?? NO_ITEMS)].map( + (item) => [item.url, item] as const, + ), + ).values(), + ), + [menus?.sidebar, menus?.adminSidebar], + ); const declaredSection = usePageSection(currentUrl); // Same "which entry does this page belong to" resolution AppTopbar uses for // the breadcrumb, so the sidebar highlight and the breadcrumb never disagree. @@ -242,6 +249,13 @@ function SidebarShell({ children, menuKey, theme, headerSlot, footerNavSlot }: S })} ))} + {menuKey === 'sidebar' && ( + + )} {footerNavSlot} diff --git a/tests/test_audit_log.py b/tests/test_audit_log.py index 4adbcb08..6ab358b5 100644 --- a/tests/test_audit_log.py +++ b/tests/test_audit_log.py @@ -201,7 +201,7 @@ async def test_invalid_pagination_returns_html( ): """View endpoint should clamp bad pagination values, never 422.""" resp = await authenticated_client.get( - "/audit_log/", + "/admin/audit-log/", params=params, follow_redirects=False, ) diff --git a/tests/test_principal_resolver_integration.py b/tests/test_principal_resolver_integration.py index 38ab337d..2ffb2c1a 100644 --- a/tests/test_principal_resolver_integration.py +++ b/tests/test_principal_resolver_integration.py @@ -48,11 +48,11 @@ async def pat_client(app_with_pat_resolver) -> AsyncGenerator[httpx.AsyncClient, async def test_bearer_token_authenticates_against_protected_view(pat_client): """Valid bearer token -> 200 on a protected view path (users admin).""" resp = await pat_client.get( - "/users/admin", + "/admin/users/", headers={"Authorization": "Bearer good"}, follow_redirects=False, ) - # /users/admin is a view route; with a valid resolver the request gets + # /admin/users/ is a view route; with a valid resolver the request gets # through AuthMiddleware (200) instead of redirecting to /users/login. assert resp.status_code == 200 From b21ebdba552788720eddf4da073ced4c1e269824 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 16:59:19 +0200 Subject: [PATCH 15/17] fix(dashboard): keep the Dashboard tile on the dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- modules/dashboard/dashboard/pages/Home.tsx | 29 ++++++++++++++----- .../users/users/admin/components/UserRow.tsx | 7 +++-- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/modules/dashboard/dashboard/pages/Home.tsx b/modules/dashboard/dashboard/pages/Home.tsx index 855eb02d..90a6e578 100644 --- a/modules/dashboard/dashboard/pages/Home.tsx +++ b/modules/dashboard/dashboard/pages/Home.tsx @@ -90,23 +90,38 @@ function Home() { * * A module that is only partly administrative mounts its admin screens * outside its own `view_prefix` (Users serves sign-in at `/users` and - * management at `/admin/users`), so both prefixes are searched — admin - * first, since that is the screen the tile is for. Without it the Users - * tile falls through to `/users/me` and opens the viewer's own profile. + * management at `/admin/users`), so both prefixes are searched. + * + * Every exact match is tried before any under-prefix guess, rather than + * exhausting one prefix before the other. Searching the admin prefix first + * 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. An exact hit is unambiguous evidence of "this is the module's + * landing screen", so it beats a guess on either prefix — which also keeps + * the Users tile on `/admin/users` rather than falling through to + * `/users/me` and opening the viewer's own profile. */ function menuTarget(url: string, adminUrl: string): string { + for (const candidate of [url, adminUrl]) { + const hit = candidate ? exactMenu(candidate) : ''; + if (hit) return hit; + } for (const candidate of [adminUrl, url]) { - const hit = candidate ? resolveUnderPrefix(candidate) : ''; + const hit = candidate ? menuUnderPrefix(candidate) : ''; if (hit) return hit; } return ''; } - function resolveUnderPrefix(url: string): string { + function exactMenu(url: string): string { + const prefix = url.replace(/\/+$/, ''); + if (!prefix) return ''; + return menuUrls.find((menuUrl) => menuUrl.replace(/\/+$/, '') === prefix) ?? ''; + } + + function menuUnderPrefix(url: string): string { const prefix = url.replace(/\/+$/, ''); if (!prefix) return ''; - const exact = menuUrls.find((menuUrl) => menuUrl.replace(/\/+$/, '') === prefix); - if (exact) return exact; return ( menuUrls.find( (menuUrl) => diff --git a/modules/users/users/admin/components/UserRow.tsx b/modules/users/users/admin/components/UserRow.tsx index 2a8c45f9..ff7980f6 100644 --- a/modules/users/users/admin/components/UserRow.tsx +++ b/modules/users/users/admin/components/UserRow.tsx @@ -78,8 +78,11 @@ export function UserRow({ user }: { user: UserListItem }) { From 6f8b791c98fa239c26755570a0ddbb752d8d5cb4 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 17:18:00 +0200 Subject: [PATCH 16/17] refactor(dashboard): reuse the shared path helpers in tile resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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_01HCFXBubRRWDDBeyqYRZ1kw --- modules/dashboard/dashboard/pages/Home.tsx | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/modules/dashboard/dashboard/pages/Home.tsx b/modules/dashboard/dashboard/pages/Home.tsx index 90a6e578..773890f1 100644 --- a/modules/dashboard/dashboard/pages/Home.tsx +++ b/modules/dashboard/dashboard/pages/Home.tsx @@ -5,6 +5,7 @@ import { SectionTitle } from '@simple-module-py/ui/components/SectionTitle'; import { StatCard } from '@simple-module-py/ui/components/StatCard'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { isUnder, samePath, trimmed } from '@simple-module-py/ui/lib/current-path'; import type { SharedProps } from '@simple-module-py/ui/types'; import { Activity, Box, Stethoscope, Users } from 'lucide-react'; import { DemoPlaceholders } from './components/DemoPlaceholders'; @@ -33,12 +34,6 @@ interface SystemInfo { health_checks: HealthCheck[]; } -/** Does `menuUrl` sit at, or below, the route prefix `owner`? */ -function isUnder(menuUrl: string, owner: string): boolean { - const normalized = menuUrl.replace(/\/+$/, ''); - return normalized === owner || normalized.startsWith(`${owner}/`); -} - interface Props { total_users: number; active_users_7d: number; @@ -72,7 +67,7 @@ function Home() { // mine" from "this entry belongs to a module mounted deeper than me". const modulePrefixes = props.system_info.modules .flatMap((m) => [m.url, m.admin_url]) - .map((url) => url.replace(/\/+$/, '')) + .map((url) => trimmed(url)) .filter(Boolean); /** @@ -114,13 +109,13 @@ function Home() { } function exactMenu(url: string): string { - const prefix = url.replace(/\/+$/, ''); + const prefix = trimmed(url); if (!prefix) return ''; - return menuUrls.find((menuUrl) => menuUrl.replace(/\/+$/, '') === prefix) ?? ''; + return menuUrls.find((menuUrl) => samePath(menuUrl, prefix)) ?? ''; } function menuUnderPrefix(url: string): string { - const prefix = url.replace(/\/+$/, ''); + const prefix = trimmed(url); if (!prefix) return ''; return ( menuUrls.find( From 6459c86d0bd71f0ab6d8daf57a28301b5ef3eeff Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 19:15:24 +0200 Subject: [PATCH 17/17] fix(hosting): let the browser tab name the page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 , 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 --- host/templates/index.html | 7 +- modules/branding/tests/test_branding.py | 12 ++- .../ui/src/components/OfflineBanner.test.tsx | 96 +++++++++++++++++++ tests/e2e/test_document_titles.py | 74 ++++++++++++++ 4 files changed, 185 insertions(+), 4 deletions(-) create mode 100644 packages/ui/src/components/OfflineBanner.test.tsx create mode 100644 tests/e2e/test_document_titles.py diff --git a/host/templates/index.html b/host/templates/index.html index 4ca6d743..87334606 100644 --- a/host/templates/index.html +++ b/host/templates/index.html @@ -4,7 +4,12 @@ <meta charset="utf-8" /> <meta name="viewport" content="width=device-width, initial-scale=1.0" /> {% set _brand = branding_head(request) %} - <title>{{ _brand.app_name }} + {# `inertia` marks this as head-managed. Inertia only replaces elements + carrying that attribute — a plain is left alone and its own + <title inertia=""> is *appended* instead, leaving two title elements. + The browser then uses the first in document order, so every page showed + the bare app name and no <Head title> ever took effect. #} + <title inertia="">{{ _brand.app_name }} {% if _brand.theme_color %}{% endif %} {% if _brand.favicon_url %}{% endif %} diff --git a/modules/branding/tests/test_branding.py b/modules/branding/tests/test_branding.py index ca27a1a9..0a81abec 100644 --- a/modules/branding/tests/test_branding.py +++ b/modules/branding/tests/test_branding.py @@ -135,18 +135,24 @@ async def test_update_persists_and_hot_swaps(app, authenticated_client: httpx.As async def test_root_template_reflects_branding( app, authenticated_client: httpx.AsyncClient ) -> None: - """The pre-hydration HTML shell carries the branded title + theme-color.""" + """The pre-hydration HTML shell carries the branded title + theme-color. + + The title carries an ``inertia`` attribute so the head manager replaces it + after hydration instead of appending a second one beside it — asserted on + the text rather than the exact markup so that attribute can change without + failing a branding test that is not about it. + """ # Default name before any change. default_page = await authenticated_client.get("/admin/branding/", follow_redirects=False) assert default_page.status_code == 200, default_page.text - assert "SimpleModule" in default_page.text + assert ">SimpleModule" in default_page.text await authenticated_client.put( "/api/branding/", json={"app_name": "Acme Corp", "primary_color": "#1A7DD1"}, ) page = await authenticated_client.get("/admin/branding/", follow_redirects=False) - assert "Acme Corp" in page.text + assert ">Acme Corp" in page.text assert '' in page.text diff --git a/packages/ui/src/components/OfflineBanner.test.tsx b/packages/ui/src/components/OfflineBanner.test.tsx new file mode 100644 index 00000000..c32865cb --- /dev/null +++ b/packages/ui/src/components/OfflineBanner.test.tsx @@ -0,0 +1,96 @@ +import { act, render, screen } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; + +vi.mock('@simple-module-py/i18n', () => ({ + useT: () => ({ + t: (key: string) => + ({ + 'host.offline.title': "You're offline", + 'host.offline.description': 'Changes may not be saved until your connection returns.', + 'host.offline.restored': 'Back online', + })[key] ?? key, + }), + keys: { + host: { + offline: { + title: 'host.offline.title', + description: 'host.offline.description', + restored: 'host.offline.restored', + }, + }, + }, +})); + +import { OfflineBanner } from './OfflineBanner'; + +/** Drive connectivity the way the browser does: flip navigator.onLine, then + * fire the event the hook actually listens for. */ +function setConnectivity(online: boolean): void { + Object.defineProperty(navigator, 'onLine', { value: online, configurable: true }); + act(() => { + window.dispatchEvent(new Event(online ? 'online' : 'offline')); + }); +} + +describe('OfflineBanner', () => { + beforeEach(() => { + vi.useFakeTimers(); + Object.defineProperty(navigator, 'onLine', { value: true, configurable: true }); + }); + + afterEach(() => { + vi.useRealTimers(); + Object.defineProperty(navigator, 'onLine', { value: true, configurable: true }); + }); + + test('renders nothing while the connection is healthy', () => { + render(); + expect(screen.queryByRole('alert')).toBeNull(); + expect(screen.queryByRole('status')).toBeNull(); + }); + + test('interrupts with an alert when the connection drops', () => { + render(); + setConnectivity(false); + // `alert`, not `status`: going offline changes what the user can do right + // now, so it should interrupt rather than wait to be read. + expect(screen.getByRole('alert')).toHaveTextContent("You're offline"); + }); + + test('confirms recovery instead of vanishing silently', () => { + render(); + setConnectivity(false); + setConnectivity(true); + // The bar that just disappears leaves the user unsure whether to retry + // what they were doing — this is the assertion QA could not make + // reliably against a real browser clock. + expect(screen.getByRole('status')).toHaveTextContent('Back online'); + }); + + test('clears the confirmation after it has been seen', () => { + render(); + setConnectivity(false); + setConnectivity(true); + expect(screen.getByRole('status')).toBeTruthy(); + + act(() => { + vi.advanceTimersByTime(3000); + }); + expect(screen.queryByRole('status')).toBeNull(); + expect(screen.queryByRole('alert')).toBeNull(); + }); + + test('a second outage re-arms the confirmation', () => { + render(); + setConnectivity(false); + setConnectivity(true); + act(() => { + vi.advanceTimersByTime(3000); + }); + + setConnectivity(false); + expect(screen.getByRole('alert')).toHaveTextContent("You're offline"); + setConnectivity(true); + expect(screen.getByRole('status')).toHaveTextContent('Back online'); + }); +}); diff --git a/tests/e2e/test_document_titles.py b/tests/e2e/test_document_titles.py new file mode 100644 index 00000000..e4540d00 --- /dev/null +++ b/tests/e2e/test_document_titles.py @@ -0,0 +1,74 @@ +"""E2E regression test: the browser tab names the page, not just the app. + +The failure mode is silent and total. Inertia's head manager only replaces +elements carrying the ``inertia`` attribute; anything else in ```` it +leaves alone and *appends* beside. The root template shipped a plain +````, so every page ended up with two title elements — the static brand +name first, Inertia's page-specific one second — and the browser takes the +first in document order. Every tab read as the bare app name, and no +``<Head title>`` anywhere in the app had any effect. + +Nothing else catches this. The title renders, the page renders, every +role-and-name assertion still passes; only reading ``document.title`` shows it. +Asserting the element *count* matters as much as the text: a second, unmanaged +title reintroduces the bug the moment one is added back to the template. +""" + +from __future__ import annotations + +import pytest +from playwright.sync_api import Page, expect + +pytestmark = pytest.mark.e2e + +# (path, the page-specific fragment its title must carry) +_TITLED_PAGES = [ + ("/dashboard/", "Dashboard"), + ("/admin", "Administration"), +] + + +def _login(page: Page, username: str, password: str) -> None: + page.goto("/") + page.get_by_role("link", name="Log in").first.click() + page.locator("#email").fill(username) + page.locator("#password").fill(password) + page.get_by_role("button", name="Log in").click() + page.wait_for_url("**/dashboard/**", timeout=15_000) + + +def test_login_page_title_names_the_page(page: Page) -> None: + """Checked signed-out too: the sign-in page is the first tab a visitor + sees, and it is served by the same template.""" + page.goto("/users/login") + expect(page).to_have_title("Login — SimpleModule", timeout=10_000) + + +def test_signed_out_page_has_exactly_one_title_element(page: Page) -> None: + """Two titles is the actual defect — the text assertions only fail + because of the ordering it produces.""" + page.goto("/users/login") + expect(page).to_have_title("Login — SimpleModule", timeout=10_000) + assert page.locator("title").count() == 1 + + +@pytest.mark.parametrize(("path", "fragment"), _TITLED_PAGES) +def test_page_title_names_the_page( + page: Page, e2e_username: str, e2e_password: str, path: str, fragment: str +) -> None: + _login(page, e2e_username, e2e_password) + page.goto(path) + # Waits for hydration: the server-rendered title is the bare app name until + # the head manager commits, so reading straight after goto races it. + expect(page).to_have_title(f"{fragment} — SimpleModule", timeout=10_000) + assert page.locator("title").count() == 1 + + +def test_error_page_title_names_the_status( + page: Page, e2e_username: str, e2e_password: str +) -> None: + """Signed in on purpose: to an anonymous visitor an unknown path is an + auth bounce, not a 404, so this would assert against the login page.""" + _login(page, e2e_username, e2e_password) + page.goto("/no-such-page-anywhere") + expect(page).to_have_title("Page Not Found — SimpleModule", timeout=10_000)