fix(dashboard): don't auto-SSO-redirect to /auth/login for password-only providers - #58852
fix(dashboard): don't auto-SSO-redirect to /auth/login for password-only providers#58852Ahmett101 wants to merge 1 commit into
Conversation
Related to the dashboard-auth auto-SSO 500 cluster (regression from merged #54846). This PR is a broader superset than the earliest-open canonical fix #54887 (middleware-only) — it also guards the direct |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (LGTM)
Fixes an SSO/auth UX bug: prevents auto-redirect to /auth/login for password-only providers (where SSO is not applicable). This is a targeted 3-file fix that correctly gates the SSO redirect on provider capability.
What looks good:
- Well-scoped to the specific auth provider case
- Clear conditional logic distinguishing SSO from password-only flows
|
Thanks for the focused dashboard-auth regression fix. This automated hermes-sweeper review found that current
Closing as implemented on main; the PR's intended behavioral guarantee is already present with regression coverage. |
Summary
When the only registered interactive auth provider was a password-only one (e.g. the basic-auth provider), the dashboard root returned HTTP 500 instead of rendering the password form.
Two compounding root causes in the dashboard auth code:
_auto_sso_responseinhermes_cli/dashboard_auth/middleware.pygot the/redirect happy whenever exactly one interactive provider was registered — and emitted a 302 to/auth/login?provider=<name>for any single provider, including password-only ones./auth/loginroute then unconditionally callsprovider.start_login().BasicAuthProvider.start_loginraisesNotImplementedErrorbecause password login is a direct POST to/auth/password-login, not an OAuth redirect chain — and the resulting 500 leaked into the unauthenticated browser session.Two narrow fixes complement each other:
_auto_sso_responseskips the auto-redirect when the single registered provider hassupports_password=True(and no OAuth session flow) — letting the request fall through to/loginwhich renders the username/password form correctly.auth_logincatchesNotImplementedErrorfromstart_loginand 302-redirects to/login(defense in depth — covers cases where someone manually types/auth/login?provider=basic, bypassing the middleware's redirect skip).Non-interactive token-only providers (e.g. drain) keep their 404 — the new
password_onlybranch only kicks in whensupports_passwordis true alongsidesupports_session=False.Changes
hermes_cli/dashboard_auth/middleware.py—_auto_sso_responseskips when the single provider advertisessupports_password=Trueand notsupports_session=Trueso the request falls through to/logininstead of hitting a 500-prone/auth/loginredirect.hermes_cli/dashboard_auth/routes.py—auth_logindistinguishes password-only providers (redirect to/login) from non-interactive token-only providers (still 404); also wraps the existingstart_logincall in a defensiveNotImplementedErrorcatch that redirects to/login.tests/hermes_cli/test_dashboard_auth_middleware.py— two new regression tests:test_password_only_provider_does_not_trigger_auto_sso_redirect— hitting/with a basic-only registry must NOT 302 to/auth/login?provider=basic.test_auth_login_redirects_to_login_when_provider_password_only— hitting/auth/login?provider=basicdirectly 302s to/login(not 404, not 500).How to Test
Pinned by stash: stashing the two source files (
hermes_cli/dashboard_auth/middleware.py+hermes_cli/dashboard_auth/routes.py) makes the second regression test fail with404 == 302— the fix is what introduces the redirect path that's now exercised.Checklist
.envnot used for non-credential settings (no config surface changed; the providers remain configured the same way)Risk & Impact
Low. The new
_auto_sso_responsegate (supports_password and not supports_session) restricts the existing auto-redirect narrowly: it skips ONLY providers that have neither OAuth nor session flow. Interactive OAuth providers (supports_session=Truewith a session-bearingstart_login) still get the same auto-redirect they do today. The defense-in-depthNotImplementedErrorcatch inauth_loginis a no-op for every existing provider — only password-only providers raise it, and the catch converts the 500 into a clean 302 to/login.Type: Bug fix
Closes: #58810