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
This commit is contained in:
Nacho Avecilla 2026-06-26 21:08:13 -03:00 committed by GitHub
parent 7a38d64a85
commit dbe734beff
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 89 additions and 6 deletions

View file

@ -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",
]

View file

@ -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: ...

View file

@ -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

View file

@ -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:

View file

@ -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:

View file

@ -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))

View file

@ -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

View file

@ -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(

View file

@ -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
# --------------------------------------------------------------------------

View file

@ -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")