Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions hermes_cli/dashboard_auth/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,18 @@ def _auto_sso_response(request: Request) -> Response | None:
# Zero → nothing to redirect to. Two+ → user must choose at /login.
return None

# A single password-only provider (e.g. the basic-auth provider) has no
# OAuth login flow to initiate — ``BasicAuthProvider.start_login``
# raises ``NotImplementedError`` and ``auth_login`` would 500 in that
# case. Skip the auto-SSO redirect so the request falls through to
# ``/login`` which renders the username/password form correctly
# (#58810).
provider = providers[0]
if getattr(provider, "supports_password", False) and not getattr(
provider, "supports_session", False
):
return None

from hermes_cli.dashboard_auth.prefix import prefix_from_request

provider = providers[0]
Expand Down
43 changes: 39 additions & 4 deletions hermes_cli/dashboard_auth/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,48 @@ async def auth_login(request: Request, provider: str, next: str = ""):
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}",
)
# Password-only providers (BasicAuthProvider) have
# ``supports_session=False`` but ``supports_password=True`` — they
# should render the /login form. Non-interactive token-only
# providers (e.g. drain) have neither, so the 404 stays.
if not getattr(p, "supports_password", False):
raise HTTPException(
status_code=404,
detail=f"Provider does not support interactive login: {provider!r}",
)
# Password-only providers have no OAuth-redirect login flow.
# Render the /login interstitial directly so the
# username/password form surfaces instead of an opaque 404.
from hermes_cli.dashboard_auth.prefix import prefix_from_request
from urllib.parse import quote

target = f"{prefix_from_request(request)}/login"
target_next = next or ""
if target_next:
target = f"{target}?next={quote(target_next, safe='')}"
return RedirectResponse(url=target, status_code=302)

try:
ls = p.start_login(redirect_uri=_redirect_uri(request))
except NotImplementedError:
# Password-only providers (e.g. ``BasicAuthProvider``) implement
# ``supports_password`` but explicitly raise ``NotImplementedError``
# from ``start_login`` because the login flow is a direct POST to
# ``/auth/password-login``, not an OAuth redirect. The auto-SSO
# path in ``_auto_sso_response`` skips these providers already,
# but a manually-typed ``/auth/login?provider=basic`` request
# would still 500 without this catch. Redirect to ``/login``
# which renders the username/password form (#58810).
from hermes_cli.dashboard_auth.prefix import prefix_from_request
from urllib.parse import quote

target = f"{prefix_from_request(request)}/login"
next_param = request.query_params.get("next", "") if hasattr(
request, "query_params"
) else ""
if next_param:
target = f"{target}?next={quote(next_param, safe='')}"
return RedirectResponse(url=target, status_code=302)
except ProviderError as e:
audit_log(
AuditEvent.LOGIN_FAILURE,
Expand Down
112 changes: 112 additions & 0 deletions tests/hermes_cli/test_dashboard_auth_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -593,3 +593,115 @@ def test_unverifiable_token_with_reachable_providers_redirects(_gated_state):
r = client.get("/api/auth/me")
assert r.status_code == 401
assert "unreachable" not in r.text.lower()


class _PasswordOnlyProvider:
"""Minimal stand-in for BasicAuthProvider for fix-regression tests.

Mirrors the shape that matters to ``_auto_sso_response``: a single
registered provider that advertises ``supports_password=True`` with
no Session/OAuth flow. ``start_login`` raises ``NotImplementedError``
exactly like ``BasicAuthProvider`` does in production (#58810).
"""

name = "basic"
display_name = "Username & Password"
supports_password = True
supports_session = False # MIRRORS BasicAuthProvider in plugin/dashboard_auth/basic

def start_login(self, *, redirect_uri): # noqa: D401
raise NotImplementedError(
"Password-only providers have no OAuth redirect; "
"POST to /auth/password-login instead."
)

def complete_login(
self, *, code, state, code_verifier, redirect_uri,
): # pragma: no cover - never invoked for basic
raise NotImplementedError

def complete_password_login(self, *, username, password, redirect_uri):
return None

def verify_session(self, *, token):
return None

def refresh_session(self, *, token):
return None

def revoke_session(self, *, token):
return None


def test_password_only_provider_does_not_trigger_auto_sso_redirect(monkeypatch):
"""A single password-only provider must NOT auto-redirect to
``/auth/login?provider=basic`` because that route calls
``start_login`` which raises ``NotImplementedError`` and returns
500 (#58810).

With the fix the unauthenticated HTML load falls through to ``/login``
which renders the username/password form correctly.
"""
from hermes_cli.dashboard_auth import clear_providers, register_provider
from fastapi.testclient import TestClient
from hermes_cli import web_server

prev = getattr(web_server.app.state, "auth_required", None)
web_server.app.state.auth_required = True
try:
clear_providers()
register_provider(_PasswordOnlyProvider())
client = TestClient(web_server.app)
# Unaauthenticated HTML load on the dashboard root MUST NOT
# bounce through ``/auth/login?provider=basic`` (that route
# raises ``NotImplementedError`` and 500s); instead it must
# fall through to ``/login`` (the password-form interstitial),
# which renders cleanly.
r = client.get("/", follow_redirects=False)
assert r.status_code == 302, (
f"Expected 302, got {r.status_code}: {r.text}"
)
location = r.headers["location"]
assert "/auth/login?provider=basic" not in location, (
f"Auto-SSO redirect to /auth/login for a password-only "
f"provider would 500 on start_login (NotImplementedError); "
f"got redirect={location!r} (#58810)"
)
assert "/login" in location, (
f"Expected fallback to /login, got {location!r} (#58810)"
)
finally:
clear_providers()
# Restore unconditionally — gate tests downstream depend on
# auth_required being reset (the prevailing ``if prev is not
# None`` guard left state at True when the suite first booted).
web_server.app.state.auth_required = prev


def test_auth_login_redirects_to_login_when_provider_password_only(monkeypatch):
"""Defense in depth: if a user types ``/auth/login?provider=basic``
directly (bypassing the auto-SSO redirect that the middleware now
skips), the route must catch ``NotImplementedError`` and redirect
to ``/login`` instead of returning 500 (#58810).
"""
from hermes_cli.dashboard_auth import clear_providers, register_provider
from fastapi.testclient import TestClient
from hermes_cli import web_server

prev = getattr(web_server.app.state, "auth_required", None)
web_server.app.state.auth_required = True
try:
clear_providers()
register_provider(_PasswordOnlyProvider())
client = TestClient(web_server.app)
r = client.get("/auth/login?provider=basic", follow_redirects=False)
assert r.status_code == 302, (
f"Expected 302 redirect to /login, got {r.status_code}: {r.text}"
)
assert "/login" in r.headers["location"], (
f"Redirect should target /login, got {r.headers['location']!r} "
"(#58810)"
)
finally:
clear_providers()
web_server.app.state.auth_required = prev