From dbe734beff0caf5e8ee2acbe4277db7f6cf84a21 Mon Sep 17 00:00:00 2001 From: Nacho Avecilla Date: Fri, 26 Jun 2026 21:08:13 -0300 Subject: [PATCH] fix(dashboard-auth): exclude non-interactive providers from interactive login surfaces (#53239) * Return None instead of erroring on drain login failure * Fix login on drain * Remove login for drained endpoints flow and clean the code * chore: drop unrelated credits changes from this PR * Remove extra comments that were not really necessary --- hermes_cli/dashboard_auth/__init__.py | 2 ++ hermes_cli/dashboard_auth/base.py | 7 +++++ hermes_cli/dashboard_auth/login_page.py | 4 +-- hermes_cli/dashboard_auth/middleware.py | 6 ++--- hermes_cli/dashboard_auth/registry.py | 9 +++++++ hermes_cli/dashboard_auth/routes.py | 10 ++++++- plugins/dashboard_auth/drain/__init__.py | 1 + .../test_dashboard_auth_middleware.py | 27 +++++++++++++++++++ tests/hermes_cli/test_dashboard_token_auth.py | 24 +++++++++++++++++ .../dashboard_auth/test_drain_provider.py | 5 ++++ 10 files changed, 89 insertions(+), 6 deletions(-) diff --git a/hermes_cli/dashboard_auth/__init__.py b/hermes_cli/dashboard_auth/__init__.py index 83a49d48834..c07b2ade6f3 100644 --- a/hermes_cli/dashboard_auth/__init__.py +++ b/hermes_cli/dashboard_auth/__init__.py @@ -25,6 +25,7 @@ from hermes_cli.dashboard_auth.registry import ( get_provider, list_providers, list_token_providers, + list_session_providers, clear_providers, ) @@ -42,5 +43,6 @@ __all__ = [ "get_provider", "list_providers", "list_token_providers", + "list_session_providers", "clear_providers", ] diff --git a/hermes_cli/dashboard_auth/base.py b/hermes_cli/dashboard_auth/base.py index 77a769cff15..8f376f35210 100644 --- a/hermes_cli/dashboard_auth/base.py +++ b/hermes_cli/dashboard_auth/base.py @@ -171,6 +171,13 @@ class DashboardAuthProvider(ABC): # future machine-credential provider drops in without core changes. supports_token: bool = False + # When True, this provider does the interactive cookie-session flow (login, + # verify, refresh). The login page, /auth/login, and the gate's + # verify/refresh loops consult only supports_session providers, so a + # token-only credential (e.g. drain) is never offered a login. Mirrors + # supports_token. + supports_session: bool = True + @abstractmethod def start_login(self, *, redirect_uri: str) -> LoginStart: ... diff --git a/hermes_cli/dashboard_auth/login_page.py b/hermes_cli/dashboard_auth/login_page.py index 6459445486b..1ccdfcba4f5 100644 --- a/hermes_cli/dashboard_auth/login_page.py +++ b/hermes_cli/dashboard_auth/login_page.py @@ -23,7 +23,7 @@ from __future__ import annotations import html -from hermes_cli.dashboard_auth import list_providers +from hermes_cli.dashboard_auth import list_session_providers # Inline minimal CSS. The dashboard's full skin lives in the React # bundle, which we deliberately do NOT load here — the login page must @@ -465,7 +465,7 @@ def render_login_html(*, next_path: str = "") -> str: validating ``next_path`` against the same-origin rules before we emit it; we still HTML-escape it as defence in depth. """ - providers = list_providers() + providers = list_session_providers() if not providers: return _EMPTY_HTML diff --git a/hermes_cli/dashboard_auth/middleware.py b/hermes_cli/dashboard_auth/middleware.py index 8b37a6388a1..f530b246be0 100644 --- a/hermes_cli/dashboard_auth/middleware.py +++ b/hermes_cli/dashboard_auth/middleware.py @@ -22,7 +22,7 @@ from typing import Awaitable, Callable from fastapi import Request from fastapi.responses import JSONResponse, RedirectResponse, Response -from hermes_cli.dashboard_auth import list_providers +from hermes_cli.dashboard_auth import list_session_providers from hermes_cli.dashboard_auth.audit import AuditEvent, audit_log from hermes_cli.dashboard_auth.base import ProviderError, RefreshExpiredError from hermes_cli.dashboard_auth.cookies import read_session_cookies @@ -230,7 +230,7 @@ async def gated_auth_middleware( # 503 — distinguishing "transient IDP outage" (don't force re-login) # from "token genuinely invalid" (fall through to refresh/relogin). unreachable_provider: str | None = None - for provider in list_providers(): + for provider in list_session_providers(): try: session = provider.verify_session(access_token=at) except ProviderError as e: @@ -344,7 +344,7 @@ def _attempt_refresh(request: Request, *, refresh_token): """ if not refresh_token: return None - for provider in list_providers(): + for provider in list_session_providers(): try: new_session = provider.refresh_session(refresh_token=refresh_token) except RefreshExpiredError: diff --git a/hermes_cli/dashboard_auth/registry.py b/hermes_cli/dashboard_auth/registry.py index e6f71744786..3f58090abf1 100644 --- a/hermes_cli/dashboard_auth/registry.py +++ b/hermes_cli/dashboard_auth/registry.py @@ -66,6 +66,15 @@ def list_token_providers() -> List[DashboardAuthProvider]: return [p for p in _providers.values() if getattr(p, "supports_token", False)] +def list_session_providers() -> List[DashboardAuthProvider]: + """Registered providers with supports_session True (interactive cookie + sessions). The login page, /auth/login, and the gate's verify/refresh loops + consult only these. Mirror of list_token_providers. + """ + with _lock: + return [p for p in _providers.values() if getattr(p, "supports_session", True)] + + def clear_providers() -> None: """Test-only: drop all registrations.""" with _lock: diff --git a/hermes_cli/dashboard_auth/routes.py b/hermes_cli/dashboard_auth/routes.py index 68ca1886ca8..d675c7858c5 100644 --- a/hermes_cli/dashboard_auth/routes.py +++ b/hermes_cli/dashboard_auth/routes.py @@ -28,6 +28,7 @@ from pydantic import BaseModel from hermes_cli.dashboard_auth import ( get_provider, list_providers, + list_session_providers, ) from hermes_cli.dashboard_auth.audit import AuditEvent, audit_log from hermes_cli.dashboard_auth.base import ( @@ -149,7 +150,9 @@ async def login_page(request: Request) -> HTMLResponse: @router.get("/api/auth/providers", name="auth_providers") async def api_auth_providers() -> Any: - providers = list_providers() + # Advertise only interactive providers; a token-only credential (e.g. drain) + # is not a sign-in option. + providers = list_session_providers() if not providers: # Q13: fail-closed when zero providers are registered. return JSONResponse( @@ -183,6 +186,11 @@ async def auth_login(request: Request, provider: str, next: str = ""): status_code=404, detail=f"Unknown provider: {provider!r}", ) + if not getattr(p, "supports_session", True): + raise HTTPException( + status_code=404, + detail=f"Provider does not support interactive login: {provider!r}", + ) try: ls = p.start_login(redirect_uri=_redirect_uri(request)) diff --git a/plugins/dashboard_auth/drain/__init__.py b/plugins/dashboard_auth/drain/__init__.py index e70117d3e9f..deac5160592 100644 --- a/plugins/dashboard_auth/drain/__init__.py +++ b/plugins/dashboard_auth/drain/__init__.py @@ -143,6 +143,7 @@ class DrainSecretProvider(DashboardAuthProvider): name = "drain-secret" display_name = "Drain Control (service credential)" supports_token = True + supports_session = False def __init__(self, *, secret: str, scope: str = "drain") -> None: # Defence in depth: construction also enforces the entropy bar, so a diff --git a/tests/hermes_cli/test_dashboard_auth_middleware.py b/tests/hermes_cli/test_dashboard_auth_middleware.py index 2e1abd4cdf1..b78b045d924 100644 --- a/tests/hermes_cli/test_dashboard_auth_middleware.py +++ b/tests/hermes_cli/test_dashboard_auth_middleware.py @@ -301,6 +301,33 @@ def test_login_unknown_provider_returns_404(gated_app): assert r.status_code == 404 +def test_login_non_interactive_provider_returns_404_not_500(gated_app): + """Regression: a token-only provider (drain) has no login flow, so + /auth/login?provider=drain-secret must 404 (not 500 on start_login) and it + must not appear in the /api/auth/providers bootstrap. + """ + import secrets + + import plugins.dashboard_auth.drain as drain_plugin + + register_provider( + drain_plugin.DrainSecretProvider(secret=secrets.token_urlsafe(48)) + ) + + r = gated_app.get( + "/auth/login?provider=drain-secret&next=%2F", follow_redirects=False + ) + assert r.status_code == 404, ( + f"drain-secret login should 404, not 500: {r.status_code} {r.text}" + ) + + bootstrap = gated_app.get("/api/auth/providers") + assert bootstrap.status_code == 200 + names = {p["name"] for p in bootstrap.json()["providers"]} + assert "drain-secret" not in names + assert "stub" in names + + def test_callback_without_pkce_cookie_returns_400(gated_app): # No prior /auth/login → no PKCE cookie. r = gated_app.get( diff --git a/tests/hermes_cli/test_dashboard_token_auth.py b/tests/hermes_cli/test_dashboard_token_auth.py index 744651c58a5..5285d853506 100644 --- a/tests/hermes_cli/test_dashboard_token_auth.py +++ b/tests/hermes_cli/test_dashboard_token_auth.py @@ -19,6 +19,8 @@ from hermes_cli.dashboard_auth import ( Session, TokenPrincipal, clear_providers, + list_providers, + list_session_providers, list_token_providers, register_provider, ) @@ -157,6 +159,28 @@ def test_list_token_providers_empty_when_none_registered(): assert list_token_providers() == [] +class _NonInteractiveProvider(_TokenProvider): + """A token-only credential with no interactive session.""" + + name = "svc-cred" + display_name = "Service Credential" + supports_session = False + + +def test_oauth_provider_defaults_supports_session_true(): + # Interactive providers participate in cookie sessions by default. + assert _OAuthOnly().supports_session is True + + +def test_list_session_providers_excludes_non_interactive(): + # Token-only providers stay out of the interactive set. Mirror of + # list_token_providers. + register_provider(_OAuthOnly()) + register_provider(_NonInteractiveProvider()) + assert {p.name for p in list_providers()} == {"oauth-only", "svc-cred"} + assert [p.name for p in list_session_providers()] == ["oauth-only"] + + # -------------------------------------------------------------------------- # Bearer extraction # -------------------------------------------------------------------------- diff --git a/tests/plugins/dashboard_auth/test_drain_provider.py b/tests/plugins/dashboard_auth/test_drain_provider.py index d20997c2ad7..371f074664f 100644 --- a/tests/plugins/dashboard_auth/test_drain_provider.py +++ b/tests/plugins/dashboard_auth/test_drain_provider.py @@ -78,6 +78,11 @@ class TestProvider: p = drain.DrainSecretProvider(secret=_strong_secret()) assert p.supports_token is True + def test_is_non_interactive(self, drain): + # Excluded from interactive surfaces via list_session_providers(). + p = drain.DrainSecretProvider(secret=_strong_secret()) + assert p.supports_session is False + def test_verify_token_accepts_matching_secret(self, drain): s = _strong_secret() p = drain.DrainSecretProvider(secret=s, scope="drain")