fix(vector): self-heal stale app passwords on auth failure
Deleted/disabled Nextcloud users left their app_passwords row in storage, so user_manager_task re-spawned their scanner every poll interval only to 401 again — an endless re-spawn/auth-failure loop (observed on tenant-blackbox-demo: ~534 respawns/3h, matching the 60s poll interval). - Delete the stored app password on a hard 401/403 in user_scanner_task (both the pre-validation and in-scan-loop paths), breaking the re-spawn loop at the source so the user-manager stops recreating the scanner. - Add a periodic credential_cleanup_task backstop (hourly) that sweeps cleanup_invalid_app_passwords for anything the per-scanner path misses. - Run the startup cleanup for all deployment modes: drop the stale `not oauth_enabled` guard so login_flow tenants (the cloud default) are covered. NOTE: login_flow startup now makes one concurrent OCS validation call per stored user before readiness. Refs Deck #198. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
60df141442
commit
3790cf6d60
@@ -130,6 +130,7 @@ from nextcloud_mcp_server.server.oauth_tools import register_oauth_tools
|
|||||||
from nextcloud_mcp_server.vector.metrics_publisher import vector_sync_metrics_task
|
from nextcloud_mcp_server.vector.metrics_publisher import vector_sync_metrics_task
|
||||||
from nextcloud_mcp_server.vector.oauth_sync import (
|
from nextcloud_mcp_server.vector.oauth_sync import (
|
||||||
ProvisionSignal,
|
ProvisionSignal,
|
||||||
|
credential_cleanup_task,
|
||||||
oauth_processor_task,
|
oauth_processor_task,
|
||||||
user_manager_task,
|
user_manager_task,
|
||||||
)
|
)
|
||||||
@@ -2067,8 +2068,13 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None =
|
|||||||
# how many per-user scanners the user-manager later starts.
|
# how many per-user scanners the user-manager later starts.
|
||||||
await _sweep_orphan_placeholders_if_enabled()
|
await _sweep_orphan_placeholders_if_enabled()
|
||||||
|
|
||||||
# Clean up stale app passwords at startup (BasicAuth mode only)
|
# Clean up stale app passwords at startup. All deployment modes
|
||||||
if not oauth_enabled:
|
# now authenticate background sync via locally-stored app
|
||||||
|
# passwords (the OAuth refresh-token path was removed), so this
|
||||||
|
# must run regardless of oauth_enabled — login_flow tenants were
|
||||||
|
# previously skipped, letting deleted-user credentials linger and
|
||||||
|
# drive an endless scanner re-spawn/401 loop (Deck #198). The
|
||||||
|
# credential_cleanup_task started below repeats it on a cadence.
|
||||||
try:
|
try:
|
||||||
removed = await token_storage.cleanup_invalid_app_passwords(
|
removed = await token_storage.cleanup_invalid_app_passwords(
|
||||||
nextcloud_host=nextcloud_host_for_sync
|
nextcloud_host=nextcloud_host_for_sync
|
||||||
@@ -2134,6 +2140,16 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None =
|
|||||||
provision_signal,
|
provision_signal,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# Periodic backstop sweep removing app passwords that no
|
||||||
|
# longer authenticate (deleted/disabled users), complementing
|
||||||
|
# the per-scanner self-heal in user_scanner_task (Deck #198).
|
||||||
|
await tg.start(
|
||||||
|
credential_cleanup_task,
|
||||||
|
token_storage,
|
||||||
|
shutdown_event,
|
||||||
|
nextcloud_host_for_sync,
|
||||||
|
)
|
||||||
|
|
||||||
# In-process consumer pool. ``run_consumers`` is a no-op for
|
# In-process consumer pool. ``run_consumers`` is a no-op for
|
||||||
# the distributed (postgres) backend — the out-of-process
|
# the distributed (postgres) backend — the out-of-process
|
||||||
# ``worker`` role consumes there. The closure binds this
|
# ``worker`` role consumes there. The closure binds this
|
||||||
|
|||||||
@@ -125,6 +125,34 @@ class UserSyncState:
|
|||||||
started_at: float = field(default_factory=time.time)
|
started_at: float = field(default_factory=time.time)
|
||||||
|
|
||||||
|
|
||||||
|
async def _remove_stale_credential(user_id: str, status_code: int) -> None:
|
||||||
|
"""Delete a user's stored app password after a hard auth failure.
|
||||||
|
|
||||||
|
A 401/403 from Nextcloud means the stored app password is no longer usable —
|
||||||
|
the user was deleted, disabled, or revoked the password. Without removing it
|
||||||
|
the credential lingers in storage and ``user_manager_task`` re-spawns this
|
||||||
|
scanner on its next poll (the user is still "provisioned"), only to fail auth
|
||||||
|
again — an endless re-spawn/401 loop that keeps hammering Nextcloud (Deck
|
||||||
|
#198). Deleting the row drops the user out of
|
||||||
|
``get_all_app_password_user_ids()`` so the scanner is not recreated.
|
||||||
|
|
||||||
|
Best-effort: a storage failure here is logged, not raised — the periodic
|
||||||
|
``credential_cleanup_task`` sweep (and the next startup sweep) are backstops.
|
||||||
|
"""
|
||||||
|
try:
|
||||||
|
storage = await _get_initialized_basic_auth_storage()
|
||||||
|
if await storage.delete_app_password(user_id):
|
||||||
|
logger.info(
|
||||||
|
"[BasicAuth] Removed stale app password for %s after HTTP %s",
|
||||||
|
user_id,
|
||||||
|
status_code,
|
||||||
|
)
|
||||||
|
except Exception as e:
|
||||||
|
logger.warning(
|
||||||
|
"[BasicAuth] Failed to remove stale app password for %s: %s", user_id, e
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
async def get_user_client_basic_auth(
|
async def get_user_client_basic_auth(
|
||||||
user_id: str,
|
user_id: str,
|
||||||
nextcloud_host: str,
|
nextcloud_host: str,
|
||||||
@@ -215,10 +243,12 @@ async def user_scanner_task(
|
|||||||
except HTTPStatusError as e:
|
except HTTPStatusError as e:
|
||||||
if e.response.status_code in (401, 403):
|
if e.response.status_code in (401, 403):
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"[BasicAuth] Credential validation failed for %s (HTTP %s), not starting scan loop",
|
"[BasicAuth] Credential validation failed for %s (HTTP %s), "
|
||||||
|
"removing stale credential and not starting scan loop",
|
||||||
user_id,
|
user_id,
|
||||||
e.response.status_code,
|
e.response.status_code,
|
||||||
)
|
)
|
||||||
|
await _remove_stale_credential(user_id, e.response.status_code)
|
||||||
return
|
return
|
||||||
raise
|
raise
|
||||||
finally:
|
finally:
|
||||||
@@ -262,10 +292,12 @@ async def user_scanner_task(
|
|||||||
status_code = e.response.status_code
|
status_code = e.response.status_code
|
||||||
if status_code in (401, 403):
|
if status_code in (401, 403):
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"[BasicAuth] Scanner auth failed for %s (HTTP %s), stopping scanner. User may need to re-provision credentials.",
|
"[BasicAuth] Scanner auth failed for %s (HTTP %s), removing stale "
|
||||||
|
"credential and stopping scanner. User must re-provision to resume sync.",
|
||||||
user_id,
|
user_id,
|
||||||
status_code,
|
status_code,
|
||||||
)
|
)
|
||||||
|
await _remove_stale_credential(user_id, status_code)
|
||||||
break
|
break
|
||||||
elif status_code == 429:
|
elif status_code == 429:
|
||||||
retry_after = min(int(e.response.headers.get("Retry-After", "60")), 300)
|
retry_after = min(int(e.response.headers.get("Retry-After", "60")), 300)
|
||||||
@@ -410,6 +442,63 @@ async def multi_user_processor_task(
|
|||||||
oauth_processor_task = multi_user_processor_task
|
oauth_processor_task = multi_user_processor_task
|
||||||
|
|
||||||
|
|
||||||
|
# Backstop sweep cadence for cleanup_invalid_app_passwords (seconds). The
|
||||||
|
# per-scanner 401 deletion (_remove_stale_credential) is the primary self-heal;
|
||||||
|
# this periodic sweep is defense-in-depth for credentials whose scanner never
|
||||||
|
# ran or whose in-scanner deletion failed. One hour keeps the per-user OCS
|
||||||
|
# validation load negligible.
|
||||||
|
CREDENTIAL_CLEANUP_INTERVAL = 3600
|
||||||
|
|
||||||
|
|
||||||
|
async def credential_cleanup_task(
|
||||||
|
storage: "RefreshTokenStorage",
|
||||||
|
shutdown_event: anyio.Event,
|
||||||
|
nextcloud_host: str,
|
||||||
|
*,
|
||||||
|
task_status: TaskStatus = anyio.TASK_STATUS_IGNORED,
|
||||||
|
) -> None:
|
||||||
|
"""Periodically remove app passwords that no longer authenticate.
|
||||||
|
|
||||||
|
Backstop for the per-scanner self-heal (``_remove_stale_credential``):
|
||||||
|
validates every stored app password against Nextcloud and deletes the ones
|
||||||
|
that return 401/403, so credentials for deleted/disabled users cannot
|
||||||
|
accumulate even if their scanner never ran (Deck #198). Runs on a fixed
|
||||||
|
cadence until ``shutdown_event`` is set. A fresh sweep already runs once at
|
||||||
|
startup (app lifespan), so this sleeps first.
|
||||||
|
"""
|
||||||
|
logger.info(
|
||||||
|
"[BasicAuth] Credential cleanup task started (interval: %ss)",
|
||||||
|
CREDENTIAL_CLEANUP_INTERVAL,
|
||||||
|
)
|
||||||
|
task_status.started()
|
||||||
|
|
||||||
|
while not shutdown_event.is_set():
|
||||||
|
# Sleep first — startup already swept; wake early on shutdown.
|
||||||
|
try:
|
||||||
|
with anyio.move_on_after(CREDENTIAL_CLEANUP_INTERVAL):
|
||||||
|
await shutdown_event.wait()
|
||||||
|
except anyio.get_cancelled_exc_class():
|
||||||
|
break
|
||||||
|
if shutdown_event.is_set():
|
||||||
|
break
|
||||||
|
|
||||||
|
try:
|
||||||
|
removed = await storage.cleanup_invalid_app_passwords(nextcloud_host)
|
||||||
|
if removed:
|
||||||
|
logger.info(
|
||||||
|
"[BasicAuth] Periodic cleanup removed %s stale app password(s): %s",
|
||||||
|
len(removed),
|
||||||
|
removed,
|
||||||
|
)
|
||||||
|
except Exception as e:
|
||||||
|
logger.warning(
|
||||||
|
"[BasicAuth] Periodic credential cleanup failed (non-fatal): %s",
|
||||||
|
format_exception_group(e),
|
||||||
|
)
|
||||||
|
|
||||||
|
logger.info("[BasicAuth] Credential cleanup task stopped")
|
||||||
|
|
||||||
|
|
||||||
async def _run_user_scanner_with_scope(
|
async def _run_user_scanner_with_scope(
|
||||||
user_id: str,
|
user_id: str,
|
||||||
cancel_scope: anyio.CancelScope,
|
cancel_scope: anyio.CancelScope,
|
||||||
|
|||||||
@@ -0,0 +1,139 @@
|
|||||||
|
"""Unit tests: self-healing removal of stale app passwords (Deck #198).
|
||||||
|
|
||||||
|
When a Nextcloud user is deleted/disabled, their stored app password keeps
|
||||||
|
returning 401. Previously the scanner just stopped, leaving the credential in
|
||||||
|
storage so ``user_manager_task`` re-spawned the scanner every poll interval — an
|
||||||
|
endless re-spawn/401 loop. The scanner now deletes the credential on a hard auth
|
||||||
|
failure, and a periodic ``credential_cleanup_task`` sweeps any that slip through.
|
||||||
|
"""
|
||||||
|
|
||||||
|
import anyio
|
||||||
|
import httpx
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from nextcloud_mcp_server.vector import oauth_sync
|
||||||
|
|
||||||
|
pytestmark = pytest.mark.unit
|
||||||
|
|
||||||
|
|
||||||
|
def _http_401() -> httpx.HTTPStatusError:
|
||||||
|
req = httpx.Request("GET", "https://cloud.example.org/ocs")
|
||||||
|
return httpx.HTTPStatusError(
|
||||||
|
"unauth", request=req, response=httpx.Response(401, request=req)
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
async def test_remove_stale_credential_deletes(mocker):
|
||||||
|
"""The helper deletes the user's app password via storage."""
|
||||||
|
storage = mocker.MagicMock()
|
||||||
|
storage.delete_app_password = mocker.AsyncMock(return_value=True)
|
||||||
|
mocker.patch.object(
|
||||||
|
oauth_sync,
|
||||||
|
"_get_initialized_basic_auth_storage",
|
||||||
|
mocker.AsyncMock(return_value=storage),
|
||||||
|
)
|
||||||
|
|
||||||
|
await oauth_sync._remove_stale_credential("ghost-user", 401)
|
||||||
|
|
||||||
|
storage.delete_app_password.assert_awaited_once_with("ghost-user")
|
||||||
|
|
||||||
|
|
||||||
|
async def test_remove_stale_credential_swallows_storage_error(mocker):
|
||||||
|
"""Best-effort: a storage failure is logged, never raised (backstops cover it)."""
|
||||||
|
storage = mocker.MagicMock()
|
||||||
|
storage.delete_app_password = mocker.AsyncMock(side_effect=RuntimeError("db down"))
|
||||||
|
mocker.patch.object(
|
||||||
|
oauth_sync,
|
||||||
|
"_get_initialized_basic_auth_storage",
|
||||||
|
mocker.AsyncMock(return_value=storage),
|
||||||
|
)
|
||||||
|
|
||||||
|
# Must not raise.
|
||||||
|
await oauth_sync._remove_stale_credential("ghost-user", 401)
|
||||||
|
|
||||||
|
|
||||||
|
async def test_scanner_removes_credential_on_prevalidation_401(mocker):
|
||||||
|
"""A 401 validating creds deletes the credential and never enters the scan
|
||||||
|
loop — so ``user_manager_task`` won't see the user as provisioned again."""
|
||||||
|
fake_client = mocker.AsyncMock()
|
||||||
|
fake_client.capabilities = mocker.AsyncMock(side_effect=_http_401())
|
||||||
|
fake_client.close = mocker.AsyncMock()
|
||||||
|
mocker.patch.object(
|
||||||
|
oauth_sync,
|
||||||
|
"get_user_client_basic_auth",
|
||||||
|
mocker.AsyncMock(return_value=fake_client),
|
||||||
|
)
|
||||||
|
storage = mocker.MagicMock()
|
||||||
|
storage.delete_app_password = mocker.AsyncMock(return_value=True)
|
||||||
|
mocker.patch.object(
|
||||||
|
oauth_sync,
|
||||||
|
"_get_initialized_basic_auth_storage",
|
||||||
|
mocker.AsyncMock(return_value=storage),
|
||||||
|
)
|
||||||
|
|
||||||
|
await oauth_sync.user_scanner_task(
|
||||||
|
"ghost-user",
|
||||||
|
mocker.MagicMock(), # send_stream — unused on the pre-validation path
|
||||||
|
anyio.Event(), # shutdown_event
|
||||||
|
anyio.Event(), # wake_event
|
||||||
|
"https://cloud.example.org",
|
||||||
|
)
|
||||||
|
|
||||||
|
storage.delete_app_password.assert_awaited_once_with("ghost-user")
|
||||||
|
|
||||||
|
|
||||||
|
async def test_scanner_removes_credential_on_scan_loop_401(mocker):
|
||||||
|
"""A 401 raised while scanning (not pre-validation) also deletes the
|
||||||
|
credential before the scanner stops."""
|
||||||
|
fake_client = mocker.AsyncMock()
|
||||||
|
fake_client.capabilities = mocker.AsyncMock(return_value={}) # pre-validation ok
|
||||||
|
fake_client.close = mocker.AsyncMock()
|
||||||
|
mocker.patch.object(
|
||||||
|
oauth_sync,
|
||||||
|
"get_user_client_basic_auth",
|
||||||
|
mocker.AsyncMock(return_value=fake_client),
|
||||||
|
)
|
||||||
|
mocker.patch.object(
|
||||||
|
oauth_sync,
|
||||||
|
"scan_user_documents",
|
||||||
|
mocker.AsyncMock(side_effect=_http_401()),
|
||||||
|
)
|
||||||
|
storage = mocker.MagicMock()
|
||||||
|
storage.delete_app_password = mocker.AsyncMock(return_value=True)
|
||||||
|
mocker.patch.object(
|
||||||
|
oauth_sync,
|
||||||
|
"_get_initialized_basic_auth_storage",
|
||||||
|
mocker.AsyncMock(return_value=storage),
|
||||||
|
)
|
||||||
|
|
||||||
|
await oauth_sync.user_scanner_task(
|
||||||
|
"ghost-user",
|
||||||
|
mocker.MagicMock(),
|
||||||
|
anyio.Event(),
|
||||||
|
anyio.Event(),
|
||||||
|
"https://cloud.example.org",
|
||||||
|
)
|
||||||
|
|
||||||
|
storage.delete_app_password.assert_awaited_once_with("ghost-user")
|
||||||
|
|
||||||
|
|
||||||
|
async def test_credential_cleanup_task_sweeps_then_stops(mocker):
|
||||||
|
"""The periodic backstop validates stored passwords via
|
||||||
|
``cleanup_invalid_app_passwords`` and exits on shutdown."""
|
||||||
|
mocker.patch.object(oauth_sync, "CREDENTIAL_CLEANUP_INTERVAL", 0)
|
||||||
|
shutdown = anyio.Event()
|
||||||
|
storage = mocker.MagicMock()
|
||||||
|
|
||||||
|
async def _cleanup(host):
|
||||||
|
shutdown.set() # stop the loop after the first sweep
|
||||||
|
return ["ghost-user"]
|
||||||
|
|
||||||
|
storage.cleanup_invalid_app_passwords = mocker.AsyncMock(side_effect=_cleanup)
|
||||||
|
|
||||||
|
await oauth_sync.credential_cleanup_task(
|
||||||
|
storage, shutdown, "https://cloud.example.org"
|
||||||
|
)
|
||||||
|
|
||||||
|
storage.cleanup_invalid_app_passwords.assert_awaited_once_with(
|
||||||
|
"https://cloud.example.org"
|
||||||
|
)
|
||||||
Reference in New Issue
Block a user