fix(dashboard-auth): keep the provider registry alive across per-home plugin-manager unloads
The dashboard auth registry is process-global, but a bundled auth provider was registered under the per-home plugin manager's scope and enrolled in that manager's reverse-order teardown. A per-home manager is unloaded routinely (profile-scoped dashboard activity, forced re-discovery), and that teardown disposed the registration — emptying the auth registry for the whole process and permanently disabling sign-in until restart. Register dashboard-auth providers in the process-global slot as persistent host-owned registrations kept out of per-home manager teardown, so a routine unload can no longer disable authentication process-wide. Registration upserts, so a forced re-discovery (e.g. a password change) still rotates the provider in place. The test-only manager reset now clears the auth registry too, since persistent registrations deliberately survive unload. Fixes #91701
This commit is contained in:
@@ -122,6 +122,44 @@ def list_session_providers() -> List[DashboardAuthProvider]:
|
||||
return [p for p in list_providers() if getattr(p, "supports_session", True)]
|
||||
|
||||
|
||||
def register_global_provider(provider: DashboardAuthProvider) -> None:
|
||||
"""Register a host-owned provider in the process-global slot (upsert).
|
||||
|
||||
The dashboard auth registry is process-global and shared across every
|
||||
profile the dashboard serves from one process, so its providers must
|
||||
outlive any single per-home plugin manager. Unlike ``register_provider``
|
||||
this always targets the global ``_providers`` map (never a per-home
|
||||
overlay) and *replaces* any same-name entry instead of raising, so a
|
||||
forced plugin re-discovery (e.g. after a password change) rotates the
|
||||
provider in place. Pairs with ``unregister_global_provider`` for teardown
|
||||
of the exact object still current (#91701).
|
||||
"""
|
||||
assert_protocol_compliance(type(provider))
|
||||
with _lock:
|
||||
_providers[provider.name] = provider
|
||||
_log.info(
|
||||
"dashboard-auth: registered global provider %r (%s)",
|
||||
provider.name, provider.display_name,
|
||||
)
|
||||
|
||||
|
||||
def unregister_global_provider(
|
||||
name: str,
|
||||
provider: DashboardAuthProvider,
|
||||
) -> bool:
|
||||
"""Remove a global provider registration if ``provider`` is still current.
|
||||
|
||||
Identity-conditional so a stale handle (whose provider was already
|
||||
replaced by a later ``register_global_provider``) never clears the live
|
||||
registration.
|
||||
"""
|
||||
with _lock:
|
||||
if _providers.get(name) is provider:
|
||||
_providers.pop(name, None)
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def clear_providers() -> None:
|
||||
"""Test-only: drop all registrations."""
|
||||
with _lock:
|
||||
|
||||
@@ -1537,10 +1537,18 @@ class PluginContext:
|
||||
kind: str,
|
||||
key: str,
|
||||
release: Callable[[], None],
|
||||
*,
|
||||
persistent: bool = False,
|
||||
) -> PluginRegistration:
|
||||
"""Record host-owned cleanup for a successful registration."""
|
||||
"""Record host-owned cleanup for a successful registration.
|
||||
|
||||
``persistent`` registrations are returned as live handles but kept
|
||||
out of the manager's reverse-order teardown, so a routine per-home
|
||||
manager unload does not dispose them (see
|
||||
:meth:`PluginManager._track_registration`).
|
||||
"""
|
||||
return self._manager._track_registration(
|
||||
self.manifest, kind, key, release
|
||||
self.manifest, kind, key, release, persistent=persistent
|
||||
)
|
||||
|
||||
def _track_replacement(
|
||||
@@ -2382,12 +2390,11 @@ class PluginContext:
|
||||
cannot crash the host. Same convention as
|
||||
``register_image_gen_provider``.
|
||||
"""
|
||||
from hermes_cli.dashboard_auth import (
|
||||
DashboardAuthProvider,
|
||||
register_provider,
|
||||
from hermes_cli.dashboard_auth import DashboardAuthProvider
|
||||
from hermes_cli.dashboard_auth.registry import (
|
||||
register_global_provider,
|
||||
unregister_global_provider,
|
||||
)
|
||||
from hermes_cli.dashboard_auth.registry import restore_registration
|
||||
from hermes_cli.dashboard_auth.registry import snapshot_registration
|
||||
|
||||
if not isinstance(provider, DashboardAuthProvider):
|
||||
logger.warning(
|
||||
@@ -2397,10 +2404,19 @@ class PluginContext:
|
||||
)
|
||||
return
|
||||
registry_name = provider.name
|
||||
scope = self._manager.scope_key
|
||||
previous = snapshot_registration(registry_name, scope=scope)
|
||||
# The dashboard auth registry is process-global — its lifetime is the
|
||||
# web server, not this per-home plugin manager. A per-home manager is
|
||||
# torn down routinely (profile-scoped dashboard activity, force
|
||||
# re-discovery), and disposing this registration on that teardown
|
||||
# emptied the auth registry for the WHOLE process, permanently
|
||||
# disabling sign-in until restart (#91701). So register it in the
|
||||
# global slot (upsert) and, crucially, keep it OUT of the manager's
|
||||
# reverse-order teardown (``persistent=True``): a per-home unload can
|
||||
# no longer clear it. The handle still disposes explicitly (identity-
|
||||
# conditional), and a forced re-discovery rotates the provider in
|
||||
# place via the upsert.
|
||||
try:
|
||||
register_provider(provider, scope=scope)
|
||||
register_global_provider(provider)
|
||||
except (TypeError, ValueError) as e:
|
||||
logger.warning(
|
||||
"Plugin '%s' failed to register dashboard-auth provider "
|
||||
@@ -2408,18 +2424,11 @@ class PluginContext:
|
||||
self.manifest.name, getattr(provider, "name", "?"), e,
|
||||
)
|
||||
return
|
||||
registered = snapshot_registration(registry_name, scope=scope)
|
||||
if registered is not provider:
|
||||
return None
|
||||
handle = self._track_replacement(
|
||||
handle = self._track(
|
||||
"dashboard_auth_provider",
|
||||
registry_name,
|
||||
slot=("dashboard_auth_provider", scope, registry_name),
|
||||
current=provider,
|
||||
previous=previous,
|
||||
restore=lambda replacement: restore_registration(
|
||||
registry_name, provider, replacement, scope=scope
|
||||
),
|
||||
lambda: unregister_global_provider(registry_name, provider),
|
||||
persistent=True,
|
||||
)
|
||||
logger.info(
|
||||
"Plugin '%s' registered dashboard-auth provider: %s (%s)",
|
||||
@@ -3478,8 +3487,18 @@ class PluginManager:
|
||||
kind: str,
|
||||
key: str,
|
||||
release: Callable[[], None],
|
||||
*,
|
||||
persistent: bool = False,
|
||||
) -> PluginRegistration:
|
||||
"""Record one successful registration under its canonical plugin key."""
|
||||
"""Record one successful registration under its canonical plugin key.
|
||||
|
||||
``persistent`` registrations (process-global host infrastructure such
|
||||
as dashboard-auth providers, whose lifetime is the server rather than
|
||||
a per-home plugin manager) are still tracked in the ownership ledger
|
||||
for attribution, but are NOT enrolled in ``_registration_order`` — so
|
||||
a routine per-home manager unload cannot dispose them (#91701). The
|
||||
returned handle still releases on explicit ``dispose()``.
|
||||
"""
|
||||
plugin_key = manifest.key or manifest.name
|
||||
registration = PluginRegistration(
|
||||
kind=kind,
|
||||
@@ -3491,7 +3510,8 @@ class PluginManager:
|
||||
[disposed]
|
||||
)
|
||||
self._ownership_ledger.setdefault(plugin_key, []).append(registration)
|
||||
self._registration_order.append(registration)
|
||||
if not persistent:
|
||||
self._registration_order.append(registration)
|
||||
return registration
|
||||
|
||||
@staticmethod
|
||||
@@ -5685,6 +5705,18 @@ def _reset_plugin_managers_for_tests() -> None:
|
||||
logger.debug("test plugin-manager unload failed", exc_info=True)
|
||||
_plugin_managers_by_home.clear()
|
||||
_plugin_manager = None
|
||||
# Dashboard-auth providers are persistent host-owned registrations that
|
||||
# deliberately survive a routine manager unload (#91701), so the "clean
|
||||
# slate" reset must drop the process-global auth registry explicitly —
|
||||
# otherwise a provider auto-registered during one test leaks into the next.
|
||||
try:
|
||||
from hermes_cli.dashboard_auth.registry import (
|
||||
clear_providers as _clear_dashboard_auth_providers,
|
||||
)
|
||||
|
||||
_clear_dashboard_auth_providers()
|
||||
except Exception:
|
||||
logger.debug("dashboard-auth registry clear failed", exc_info=True)
|
||||
|
||||
|
||||
def has_enabled_agent_plugin_mcp(raw_config: Mapping[str, Any]) -> bool:
|
||||
|
||||
@@ -11,7 +11,9 @@ from hermes_cli.dashboard_auth import clear_providers, get_provider
|
||||
from hermes_cli.dashboard_auth.base import (
|
||||
DashboardAuthProvider, LoginStart, Session,
|
||||
)
|
||||
from hermes_cli.plugins import PluginContext, PluginManifest
|
||||
from hermes_cli.plugins import PluginContext, PluginManager, PluginManifest
|
||||
from hermes_cli.dashboard_auth import registry as _auth_registry
|
||||
from hermes_constants import hermes_home_key
|
||||
|
||||
|
||||
class _Stub(DashboardAuthProvider):
|
||||
@@ -80,3 +82,90 @@ def test_plugin_ctx_silently_ignores_non_provider(caplog):
|
||||
and "DashboardAuthProvider" in rec.message
|
||||
for rec in caplog.records
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #91701: a dashboard-auth provider is process-global host infrastructure. A
|
||||
# per-home plugin manager is torn down routinely (profile-scoped dashboard
|
||||
# activity, force re-discovery); that teardown must NOT empty the auth
|
||||
# registry and lock the whole process out of sign-in.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class _Basic(DashboardAuthProvider):
|
||||
name = "basic"
|
||||
display_name = "Basic"
|
||||
|
||||
def __init__(self, tag: str = "a") -> None:
|
||||
self.tag = tag
|
||||
|
||||
def start_login(self, *, redirect_uri):
|
||||
return LoginStart(redirect_url="x", cookie_payload={})
|
||||
|
||||
def complete_login(self, *, code, state, code_verifier, redirect_uri):
|
||||
return Session("u", "e", "n", "o", "basic", 0, "a", "r")
|
||||
|
||||
def verify_session(self, *, access_token):
|
||||
return None
|
||||
|
||||
def refresh_session(self, *, refresh_token):
|
||||
return None
|
||||
|
||||
def revoke_session(self, *, refresh_token):
|
||||
return None
|
||||
|
||||
|
||||
def _real_ctx() -> tuple[PluginManager, PluginContext]:
|
||||
manager = PluginManager(scope_key=hermes_home_key())
|
||||
manifest = PluginManifest(name="basic", version="0.0.1", kind="backend")
|
||||
return manager, PluginContext(manifest=manifest, manager=manager)
|
||||
|
||||
|
||||
def test_auth_provider_registers_globally_not_in_home_overlay():
|
||||
"""Registered in the process-global slot so every profile scope sees it."""
|
||||
manager, ctx = _real_ctx()
|
||||
ctx.register_dashboard_auth_provider(_Basic())
|
||||
assert "basic" in _auth_registry._providers
|
||||
assert "basic" not in _auth_registry._scoped_providers.get(
|
||||
manager.scope_key, {}
|
||||
)
|
||||
assert [p.name for p in _auth_registry.list_session_providers()] == ["basic"]
|
||||
|
||||
|
||||
def test_auth_provider_survives_per_home_manager_unload():
|
||||
"""Regression for #91701: routine per-home unload must not disable auth."""
|
||||
manager, ctx = _real_ctx()
|
||||
ctx.register_dashboard_auth_provider(_Basic())
|
||||
assert get_provider("basic") is not None
|
||||
|
||||
# The exact teardown discover_and_load(force=True) / profile-scoped
|
||||
# activity drives; before the fix this emptied the registry permanently.
|
||||
manager.unload()
|
||||
|
||||
assert get_provider("basic") is not None, (
|
||||
"auth provider was disposed by a per-home plugin-manager unload"
|
||||
)
|
||||
assert [p.name for p in _auth_registry.list_session_providers()] == ["basic"]
|
||||
|
||||
|
||||
def test_auth_provider_kept_out_of_manager_teardown_order():
|
||||
"""Persistent registration is not enrolled in reverse-order teardown."""
|
||||
manager, ctx = _real_ctx()
|
||||
ctx.register_dashboard_auth_provider(_Basic())
|
||||
assert manager._registration_order == []
|
||||
# Still attributed to the plugin for `hermes plugins list`.
|
||||
assert "basic" in manager._ownership_ledger
|
||||
|
||||
|
||||
def test_auth_provider_re_register_rotates_in_place():
|
||||
"""A forced re-discovery (e.g. password change) upserts the new provider."""
|
||||
manager, ctx = _real_ctx()
|
||||
old = _Basic("old")
|
||||
new = _Basic("new")
|
||||
stale = ctx.register_dashboard_auth_provider(old)
|
||||
ctx.register_dashboard_auth_provider(new)
|
||||
assert get_provider("basic") is new
|
||||
|
||||
# The superseded handle is identity-conditional: disposing it is a no-op.
|
||||
stale.dispose()
|
||||
assert get_provider("basic") is new
|
||||
|
||||
Reference in New Issue
Block a user