From cd3fb4e96bba195b43dc2357e97bf666d99649b2 Mon Sep 17 00:00:00 2001 From: RaresKeY <158580472+RaresKeY@users.noreply.github.com> Date: Wed, 10 Jun 2026 17:24:27 +0300 Subject: [PATCH] fix(auth): fail closed when deleting user tokens fails (#3733) --- core/auth.py | 28 ++++++++------ routes/auth_routes.py | 25 ++++++++---- tests/test_auth_config_lock_concurrency.py | 38 +++++++++++++++++++ ...est_delete_user_invalidates_token_cache.py | 24 ++++++++++++ tests/test_delete_user_revokes_api_tokens.py | 18 +++++++++ 5 files changed, 114 insertions(+), 19 deletions(-) diff --git a/core/auth.py b/core/auth.py index 5db2fed4c..11f38cd5f 100644 --- a/core/auth.py +++ b/core/auth.py @@ -244,6 +244,22 @@ class AuthManager: return False if not self.users.get(requesting_user, {}).get("is_admin"): return False + # Revoke API bearer tokens before removing the auth row. The bearer + # path authenticates from ApiToken rows and does not require the + # owner to still exist, so a successful delete must not leave active + # rows behind. If the token store is unavailable, fail closed and + # keep the user/session state intact so the admin can retry. + try: + from core.database import get_db_session, ApiToken + with get_db_session() as db: + removed_tokens = db.query(ApiToken).filter(ApiToken.owner == username).delete() + if removed_tokens: + logger.info( + f"Revoked {removed_tokens} API token(s) owned by deleted user '{username}'" + ) + except Exception: + logger.warning(f"Failed to revoke API tokens for deleted user '{username}'") + return False del self._config["users"][username] self._save() # Purge all sessions belonging to this user. validate_token doesn't @@ -258,18 +274,6 @@ class AuthManager: revoked += 1 if revoked: self._save_sessions() - # Also revoke API bearer tokens owned by this user. The bearer auth - # path authenticates straight against ApiToken rows and never - # re-checks that the owner still exists, so leaving the rows behind - # would let a deleted user keep full API access indefinitely. - try: - from core.database import get_db_session, ApiToken - with get_db_session() as db: - removed = db.query(ApiToken).filter(ApiToken.owner == username).delete() - if removed: - logger.info(f"Revoked {removed} API token(s) owned by deleted user '{username}'") - except Exception: - logger.warning(f"Failed to revoke API tokens for deleted user '{username}'") logger.info(f"Deleted user '{username}' (by {requesting_user}); revoked {revoked} active session(s)") return True diff --git a/routes/auth_routes.py b/routes/auth_routes.py index c20860892..853958d35 100644 --- a/routes/auth_routes.py +++ b/routes/auth_routes.py @@ -473,7 +473,23 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: user = _get_current_user(request) if not user or not auth_manager.is_admin(user): raise HTTPException(403, "Admin only") - ok = auth_manager.delete_user(body.username, user) + + def _invalidate_api_token_cache(): + try: + invalidator = getattr(request.app.state, "invalidate_token_cache", None) + if invalidator: + invalidator() + except Exception: + pass + + try: + ok = auth_manager.delete_user(body.username, user) + except Exception: + # delete_user can touch ApiToken rows before a later auth-store write + # fails. Dirty the bearer cache anyway so a partial token purge does + # not leave already-cached tokens authenticating until restart. + _invalidate_api_token_cache() + raise if not ok: raise HTTPException(400, "Cannot delete user") # delete_user removes the user's ApiToken rows, but the bearer-auth @@ -481,12 +497,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: # rebuilds when flagged dirty. Without this, a deleted user's already # cached token keeps authenticating until some other token op or a # restart clears the cache. Mirror what the token routes do. - try: - invalidator = getattr(request.app.state, "invalidate_token_cache", None) - if invalidator: - invalidator() - except Exception: - pass + _invalidate_api_token_cache() return {"ok": True} # ---- Feature visibility (admin-managed) ---- diff --git a/tests/test_auth_config_lock_concurrency.py b/tests/test_auth_config_lock_concurrency.py index f5cc8a18c..34232b9e2 100644 --- a/tests/test_auth_config_lock_concurrency.py +++ b/tests/test_auth_config_lock_concurrency.py @@ -8,6 +8,9 @@ with missing users or assertion errors. import json import threading import time +import contextlib +import sys +import types from concurrent.futures import ThreadPoolExecutor, as_completed import pytest @@ -15,6 +18,41 @@ import pytest from tests.helpers.import_state import clear_module +class _OwnerColumn: + def __eq__(self, other): + return ("owner ==", other) + + +class _FakeApiToken: + owner = _OwnerColumn() + + +class _FakeQuery: + def filter(self, *_conds): + return self + + def delete(self, *args, **kwargs): + return 0 + + +class _FakeSession: + def query(self, model): + assert model is _FakeApiToken + return _FakeQuery() + + +@pytest.fixture(autouse=True) +def _stub_api_token_purge(monkeypatch): + @contextlib.contextmanager + def _fake_db_session(): + yield _FakeSession() + + db_stub = types.ModuleType("core.database") + db_stub.get_db_session = _fake_db_session + db_stub.ApiToken = _FakeApiToken + monkeypatch.setitem(sys.modules, "core.database", db_stub) + + def _fresh_auth_manager(tmp_path): clear_module("core.auth") from core.auth import AuthManager diff --git a/tests/test_delete_user_invalidates_token_cache.py b/tests/test_delete_user_invalidates_token_cache.py index c9cb79a5e..91be50e93 100644 --- a/tests/test_delete_user_invalidates_token_cache.py +++ b/tests/test_delete_user_invalidates_token_cache.py @@ -36,6 +36,17 @@ def _auth_manager(delete_result): ) +def _auth_manager_raising(): + def _delete_user(_username, _requesting_user): + raise RuntimeError("auth save failed after token purge") + + return types.SimpleNamespace( + get_username_for_token=lambda token: "admin", + is_admin=lambda user: True, + delete_user=_delete_user, + ) + + def test_successful_delete_invalidates_cache(): invalidations = [] router = setup_auth_routes(_auth_manager(delete_result=True)) @@ -56,3 +67,16 @@ def test_refused_delete_does_not_invalidate_cache(): raised = True assert raised, "a refused delete should raise (HTTP 400)" assert invalidations == [], "a refused delete must not touch the token cache" + + +def test_delete_exception_invalidates_cache_for_partial_token_purge(): + invalidations = [] + router = setup_auth_routes(_auth_manager_raising()) + handler = _handler(router) + try: + asyncio.run(handler(DeleteUserRequest(username="bob"), _fake_request(invalidations))) + raised = False + except RuntimeError: + raised = True + assert raised, "delete_user exception should still propagate" + assert invalidations == [True], "partial token purge must dirty the bearer cache" diff --git a/tests/test_delete_user_revokes_api_tokens.py b/tests/test_delete_user_revokes_api_tokens.py index dab753ff0..52a7d55af 100644 --- a/tests/test_delete_user_revokes_api_tokens.py +++ b/tests/test_delete_user_revokes_api_tokens.py @@ -114,3 +114,21 @@ def test_refused_delete_leaves_tokens_alone(manager, db_calls): def test_unknown_user_leaves_tokens_alone(manager, db_calls): assert manager.delete_user("ghost", "admin") is False assert db_calls == [] + + +def test_delete_user_fails_closed_when_api_token_purge_fails(manager, monkeypatch): + token = manager.create_session("bob", "secret-bob-pw") + + @contextlib.contextmanager + def _failing_db_session(): + raise RuntimeError("database unavailable") + yield + + db_stub = types.ModuleType("core.database") + db_stub.get_db_session = _failing_db_session + db_stub.ApiToken = _FakeApiToken + monkeypatch.setitem(sys.modules, "core.database", db_stub) + + assert manager.delete_user("bob", "admin") is False + assert "bob" in manager.users + assert manager.validate_token(token) is True