fix(auth): fail closed when deleting user tokens fails (#3733)

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
 

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) ----

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

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"

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