fix: dashboard auth crash with NotImplementedError for password-only providers - #721
Open
hashbender wants to merge 1 commit into
Open
hashbender wants to merge 1 commit into
hashbender wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: Dashboard auth crashes with NotImplementedError for password-only providers
Summary
When a dashboard is configured with only a
BasicAuthProvider(username/password auth, no OAuth IDP), every unauthenticated request crashes with a 500NotImplementedErrorinstead of showing the login page. The user can never reach the dashboard.Root cause: The auto-SSO middleware (
_auto_sso_response) and the/auth/loginroute both assume the sole registered provider supports the OAuth redirect flow (start_login).BasicAuthProvidersetssupports_password = Trueand intentionally raisesNotImplementedErrorfromstart_login— it has no OAuth flow, only a credential form. The code never checkssupports_passwordbefore callingstart_login, so it blindly invokes a method that raises.Trace:
gated_auth_middleware→ no cookie → calls_auto_sso_response()_auto_sso_responsesees exactly 1 provider (basic) → auto-redirects to/auth/login?provider=basic/auth/loginroute callsp.start_login(redirect_uri=...)BasicAuthProvider.start_login()→ raisesNotImplementedErrorThe fix
Two changes, both checking
supports_passwordbefore attempting the OAuth path:1.
hermes_cli/dashboard_auth/middleware.py—_auto_sso_responseWhen the sole provider is password-only, return
None(fall through to the normal/loginredirect). The login page already renders a credential form forsupports_passwordproviders.2.
hermes_cli/dashboard_auth/routes.py—auth_loginWhen someone hits
/auth/login?provider=basicdirectly (e.g. bookmark, manual URL), redirect to/logininstead of callingstart_loginand crashing.if not getattr(p, "supports_session", True): raise HTTPException( status_code=404, detail=f"Provider does not support interactive login: {provider!r}", ) + if getattr(p, "supports_password", False): + # Password-only providers have no OAuth redirect flow. + # Redirect to the login page which renders the credential form. + from urllib.parse import quote + target = "/login" + if next: + target = f"{target}?next={quote(next, safe='')}" + return RedirectResponse(url=target, status_code=302) + try: ls = p.start_login(redirect_uri=_redirect_uri(request)) except ProviderError as e:Why check
supports_password(not provider name)?Both checks use
getattr(p, "supports_password", False)— the same flag the login page already uses to decide between rendering a credential form vs. an OAuth button. This keeps the fix generic: any future password-only provider benefits automatically, with no special-casing by name.Impact
/auth/password-login(the existing, already-working path), gets session cookies.supports_passwordisFalsefor OAuth providers, so both new checks are no-ops._auto_sso_responsealready returnsNonewhenlen(providers) != 1.Testing
Reproduce the original crash with a minimal BasicAuthProvider-only setup:
# Start dashboard with only basic auth configured HERMES_DASHBOARD_BASIC_AUTH_USERNAME=admin \ HERMES_DASHBOARD_BASIC_AUTH_PASSWORD=secret \ python -m hermes_cli dashboard --host 0.0.0.0 --port 9119 --no-openBefore fix:
curl http://localhost:9119/→ 500NotImplementedError: BasicAuthProvider is password-onlyAfter fix:
curl http://localhost:9119/→ 302 →/login(renders credential form)Backwards compatibility
No config changes, no API changes, no migration. Existing OAuth-only and mixed deployments are completely unaffected — the new
supports_passwordchecks are no-ops for any provider wheresupports_passwordisFalse(the default).Mirror-of: NousResearch#56886
NousResearch#56886