fix(dashboard-auth): skip password-only providers in auto-SSO redirect (#58237) - #58247
fix(dashboard-auth): skip password-only providers in auto-SSO redirect (#58237)#58247PRATHAMESH75 wants to merge 1 commit into
Conversation
When only a password-only auth provider (e.g. basic auth) is registered, an unauthenticated HTML load auto-initiated the OAuth redirect to /auth/login, whose start_login() is a NotImplementedError stub for such providers — returning a 500 instead of the login form. Filter password-only providers out of the auto-SSO candidate list. When none remain, _auto_sso_response returns None and the middleware falls through to the /login credential form, which is the correct destination. Fixes NousResearch#58237
Duplicate of #54887 (earliest-open canonical fix). This PR applies the byte-for-byte same |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Summary
Fixes auto-SSO redirect to skip password-only providers, which have no OAuth flow and would cause a silent redirect loop.
Looks Good
- Targeted fix: 2 files, 31 additions
- Clear inline comment explaining why password-only providers are excluded
- Test added for the regression case
- Correct filtering logic: not p.supports_password
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (LGTM — token read-only, formal approval deferred to maintainer)
Documentation fix for himalaya email skill: updates flag add/remove syntax for v1.2.0 which changed from --flag <name> to positional arguments. Also documents batch flag operations. The old syntax is removed in v1.2.0 and would cause errors.
Looks Good
- Accurate documentation update for v1.2.0 syntax change
- Documents both single and batch flag operations
Reviewed by Hermes Agent
|
Thanks for the focused regression report and test coverage. This is an automated hermes-sweeper review; the reported password-only auto-SSO failure is already fixed on current
Closing as implemented on main. |
What does this PR do?
Fixes a 500 Internal Server Error when the dashboard is secured with only a password-only auth provider (e.g. basic auth) and visited unauthenticated.
_auto_sso_response()inhermes_cli/dashboard_auth/middleware.pyauto-initiates the portal OAuth redirect when exactly one interactive (session) provider is registered. It filtered candidates vialist_session_providers()(which only checkssupports_session=True), so a password-only provider like basic auth was treated as a redirect provider. The middleware then 302'd to/auth/login?provider=basic, whosestart_login()is aNotImplementedErrorstub for password-only providers — yielding a 500 instead of the login form.The fix drops password-only providers (
supports_password=True) from the auto-SSO candidate list. When none remain the function returnsNone, and the middleware falls through to its ordinary_unauth_response, which already redirects HTML navigation to/login(the credential form) and returns 401 JSON for/api/*. Only the auto-SSO fast-path had the bug.Related Issue
Fixes #58237
Type of Change
Changes Made
hermes_cli/dashboard_auth/middleware.py— in_auto_sso_response, filter outsupports_passwordproviders before the single-provider auto-redirect check (they have no OAuthstart_loginto initiate).tests/hermes_cli/test_dashboard_auth_password_login.py— addTestAutoSsoSkipsPasswordOnlyProvider: with only a password-only provider registered, an unauth HTML load falls back to/login(not/auth/login) and does not 500.How to Test
Result: 98 passed. The two new tests fail against
upstream/main(reproducing theNotImplementedError500 fromstart_login()) and pass with this change.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A