diff --git a/routes/auth_routes.py b/routes/auth_routes.py index 6e0ae8a5a..e67a4758f 100644 --- a/routes/auth_routes.py +++ b/routes/auth_routes.py @@ -305,6 +305,19 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: if not ok: raise HTTPException(400, "Cannot rename user") + def _rollback_auth_rename() -> bool: + # On self-rename the admin session has already moved to the new + # username, so the rollback must authenticate as the new user. + rollback_user = new_username if user == old_username else user + try: + return bool(auth_manager.rename_user(new_username, old_username, rollback_user)) + except Exception as rollback_err: + logger.error( + "Failed to roll back auth rename %s -> %s after owner migration failure: %s", + new_username, old_username, rollback_err, + ) + return False + # Usernames are ownership keys for user data. Rename the common # owner-scoped DB rows so the account keeps access to its sessions, # docs, email accounts, tasks, etc. @@ -330,6 +343,11 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: db.close() except Exception as e: logger.error("Failed to rename owner references %s -> %s: %s", old_username, new_username, e) + if not _rollback_auth_rename(): + logger.error( + "Auth rename %s -> %s could not be rolled back after owner migration failure", + old_username, new_username, + ) raise HTTPException(500, "Failed to rename user data") # Per-user prefs are JSON-backed, not SQL-backed. diff --git a/tests/test_rename_user_owner_sync.py b/tests/test_rename_user_owner_sync.py index 1de14f31a..24e1fb67c 100644 --- a/tests/test_rename_user_owner_sync.py +++ b/tests/test_rename_user_owner_sync.py @@ -26,6 +26,7 @@ from types import SimpleNamespace from unittest.mock import MagicMock import pytest +from fastapi import HTTPException def _route(router, name): @@ -63,18 +64,68 @@ def rename_endpoint(monkeypatch, tmp_path): return _route(ar.setup_auth_routes(am), "rename_user"), am, tmp_path -def _request(tmp_path, session_manager=None): +def _request(tmp_path, session_manager=None, token="t"): state = SimpleNamespace( invalidate_token_cache=lambda: None, session_manager=session_manager, ) return SimpleNamespace( - cookies={"odysseus_session": "t"}, + cookies={"odysseus_session": token}, app=SimpleNamespace(state=state), state=SimpleNamespace(current_user="admin"), ) +def _auth_manager_for_rollback_test(monkeypatch, tmp_path): + import core.auth as auth_mod + + monkeypatch.setattr(auth_mod, "_hash_password", lambda password: f"hash:{password}") + monkeypatch.setattr(auth_mod, "_verify_password", lambda password, hashed: hashed == f"hash:{password}") + + am = auth_mod.AuthManager(str(tmp_path / "auth.json")) + assert am.create_user("admin", "pw-123456", is_admin=True) is True + assert am.create_user("alice", "pw-123456") is True + return am + + +def _force_sql_owner_migration_failure(monkeypatch): + import core.database as cdb + + class OwnerModel: + owner = "owner" + + class FailingQuery: + def filter(self, *_args, **_kwargs): + return self + + def update(self, *_args, **_kwargs): + raise RuntimeError("forced owner migration failure") + + class FailingSession: + def __init__(self): + self.rolled_back = False + self.closed = False + + def query(self, _model): + return FailingQuery() + + def rollback(self): + self.rolled_back = True + + def close(self): + self.closed = True + + db = FailingSession() + monkeypatch.setattr(cdb, "SessionLocal", lambda: db) + monkeypatch.setattr( + cdb, + "Base", + SimpleNamespace(registry=SimpleNamespace(mappers=[SimpleNamespace(class_=OwnerModel)])), + raising=False, + ) + return db + + # --------------------------------------------------------------------------- # 1. In-memory session cache # --------------------------------------------------------------------------- @@ -365,7 +416,68 @@ def test_rename_usage_keys_case_insensitive(rename_endpoint): # --------------------------------------------------------------------------- -# 5. P1 regression: rejected auth rename must not mutate file-backed stores +# 5. Rollback: auth rename must be restored if SQL owner migration fails +# --------------------------------------------------------------------------- + +def test_owner_migration_failure_rolls_back_auth_rename(monkeypatch, tmp_path): + import routes.auth_routes as ar + + db = _force_sql_owner_migration_failure(monkeypatch) + am = _auth_manager_for_rollback_test(monkeypatch, tmp_path) + admin_token = am.create_session_trusted("admin") + alice_token = am.create_session_trusted("alice") + endpoint = _route(ar.setup_auth_routes(am), "rename_user") + + with pytest.raises(HTTPException) as exc: + asyncio.run( + endpoint( + "alice", + SimpleNamespace(username="alice2"), + _request(tmp_path, token=admin_token), + ) + ) + + assert exc.value.status_code == 500 + assert db.rolled_back is True + assert db.closed is True + assert "alice" in am.users + assert "alice2" not in am.users + assert am.get_username_for_token(alice_token) == "alice" + saved_users = json.loads((tmp_path / "auth.json").read_text(encoding="utf-8"))["users"] + assert "alice" in saved_users + assert "alice2" not in saved_users + + +def test_self_rename_owner_migration_failure_rolls_back_auth_session(monkeypatch, tmp_path): + import routes.auth_routes as ar + + db = _force_sql_owner_migration_failure(monkeypatch) + am = _auth_manager_for_rollback_test(monkeypatch, tmp_path) + admin_token = am.create_session_trusted("admin") + endpoint = _route(ar.setup_auth_routes(am), "rename_user") + + with pytest.raises(HTTPException) as exc: + asyncio.run( + endpoint( + "admin", + SimpleNamespace(username="chief"), + _request(tmp_path, token=admin_token), + ) + ) + + assert exc.value.status_code == 500 + assert db.rolled_back is True + assert db.closed is True + assert "admin" in am.users + assert "chief" not in am.users + assert am.get_username_for_token(admin_token) == "admin" + saved_users = json.loads((tmp_path / "auth.json").read_text(encoding="utf-8"))["users"] + assert "admin" in saved_users + assert "chief" not in saved_users + + +# --------------------------------------------------------------------------- +# 6. P1 regression: rejected auth rename must not mutate file-backed stores # --------------------------------------------------------------------------- def test_rejected_rename_does_not_mutate_files(monkeypatch, tmp_path):