From 2fdb4813dbaa0a2258634299a0344c5e087edffc Mon Sep 17 00:00:00 2001 From: Ashvin <76151462+ashvinctrl@users.noreply.github.com> Date: Tue, 9 Jun 2026 13:49:45 +0530 Subject: [PATCH] fix(auth): sync file-backed and in-memory owner caches on user rename (#3397) The DB owner-rename loop in rename_user patched every SQL column named owner, but three non-SQL stores were left behind: 1. session_manager.sessions -- in-memory Session objects carry s.owner set at server-boot time. get_sessions_for_user() does an exact s.owner == username check, so the renamed user chat sidebar goes empty until a server restart. 2. data/deep_research/*.json -- each completed research report is a standalone JSON file with an owner field. research_routes filters by d.get(owner) == user, making every report invisible to the renamed user. 3. data/memory.json -- a flat JSON array; each entry carries an owner field. memory_manager.load(owner=user) filters on it, so all memories vanish from the memory panel. Fix: after the SQL loop, patch all three: - iterate sm.sessions and update owner in-place (exposed via app.state) - walk data/deep_research/*.json and rewrite owner with atomic_write_json - update matching entries in memory.json with atomic_write_json All three use the same case-insensitive lower() comparison the SQL loop already uses. Each step is independently wrapped so a single failure does not abort the others or the rename itself. Fixes #3362 --- app.py | 1 + routes/auth_routes.py | 105 +++++++- tests/test_rename_user_owner_sync.py | 384 +++++++++++++++++++++++++++ 3 files changed, 485 insertions(+), 5 deletions(-) create mode 100644 tests/test_rename_user_owner_sync.py diff --git a/app.py b/app.py index 22b63cc82..03e13f60a 100644 --- a/app.py +++ b/app.py @@ -472,6 +472,7 @@ components = initialize_managers(BASE_DIR, rag_manager) session_manager = components["session_manager"] from src.assistant_log import set_session_manager as _set_asst_sm _set_asst_sm(session_manager) +app.state.session_manager = session_manager memory_manager = components["memory_manager"] memory_vector = components.get("memory_vector") upload_handler = components["upload_handler"] diff --git a/routes/auth_routes.py b/routes/auth_routes.py index 9379bced8..c20860892 100644 --- a/routes/auth_routes.py +++ b/routes/auth_routes.py @@ -7,7 +7,13 @@ import asyncio import logging import os +import json +import re +from pathlib import Path + +from core.atomic_io import atomic_write_json, atomic_write_text from core.auth import AuthManager +from src.constants import DEEP_RESEARCH_DIR, MEMORY_FILE, SKILLS_DIR from src.rate_limiter import RateLimiter from src.settings_scrub import scrub_settings from src.settings import ( @@ -291,9 +297,17 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: if new_username in auth_manager.users: raise HTTPException(409, "Username already taken") + # Gate on auth first. Every mutation below is contingent on this + # succeeding — doing it last meant a rejected rename (e.g. reserved + # username) left file-backed owner fields already rewritten with no + # way to roll them back. + ok = auth_manager.rename_user(old_username, new_username, user) + if not ok: + raise HTTPException(400, "Cannot rename user") + # Usernames are ownership keys for user data. Rename the common - # owner-scoped DB rows before changing auth so the account keeps - # access to its sessions, docs, email accounts, tasks, etc. + # owner-scoped DB rows so the account keeps access to its sessions, + # docs, email accounts, tasks, etc. try: from sqlalchemy import func from core.database import Base, SessionLocal @@ -335,9 +349,90 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: except Exception as e: logger.warning("Failed to rename user prefs %s -> %s: %s", old_username, new_username, e) - ok = auth_manager.rename_user(old_username, new_username, user) - if not ok: - raise HTTPException(400, "Cannot rename user") + # deep_research: each completed report is a standalone JSON file with + # an `owner` field. research_routes filters by d.get("owner") == user, + # so a stale owner makes every report invisible to the renamed user. + try: + dr_dir = Path(DEEP_RESEARCH_DIR) + if dr_dir.is_dir(): + for p in dr_dir.glob("*.json"): + try: + d = json.loads(p.read_text(encoding="utf-8")) + if str(d.get("owner", "")).strip().lower() == old_username: + d["owner"] = new_username + atomic_write_json(str(p), d) + except Exception as err: + logger.warning("Failed to update research owner in %s: %s", p.name, err) + except Exception as e: + logger.warning("Failed to rename research owner references %s -> %s: %s", old_username, new_username, e) + + # memory.json: a flat JSON array where each entry carries an `owner` + # field. memory_manager.load(owner=user) filters on it, so stale + # entries disappear from the memory panel. + try: + if os.path.isfile(MEMORY_FILE): + with open(MEMORY_FILE, encoding="utf-8") as fh: + entries = json.loads(fh.read()) + if isinstance(entries, list): + changed = False + for entry in entries: + if isinstance(entry, dict) and str(entry.get("owner", "")).strip().lower() == old_username: + entry["owner"] = new_username + changed = True + if changed: + atomic_write_json(MEMORY_FILE, entries) + except Exception as e: + logger.warning("Failed to rename memory.json owner references %s -> %s: %s", old_username, new_username, e) + + # skills: SKILL.md frontmatter carries owner: ; the usage + # sidecar (_usage.json) keys entries as owner::skill-name. Both must + # be updated or the renamed user's Skills panel goes empty. + try: + skills_root = Path(SKILLS_DIR) + if skills_root.is_dir(): + _owner_re = re.compile( + r'(?m)^(owner:\s*)' + re.escape(old_username) + r'\s*$' + ) + for p in skills_root.rglob("SKILL.md"): + try: + text = p.read_text(encoding="utf-8") + new_text = _owner_re.sub(r'\g<1>' + new_username, text) + if new_text != text: + atomic_write_text(str(p), new_text) + except Exception as err: + logger.warning("Failed to update skill owner in %s: %s", p, err) + usage_path = skills_root / "_usage.json" + if usage_path.is_file(): + try: + usage = json.loads(usage_path.read_text(encoding="utf-8")) + if isinstance(usage, dict): + prefix = old_username + "::" + new_usage = {} + changed = False + for k, v in usage.items(): + if k.startswith(prefix): + new_usage[new_username + "::" + k[len(prefix):]] = v + changed = True + else: + new_usage[k] = v + if changed: + atomic_write_json(str(usage_path), new_usage) + except Exception as err: + logger.warning("Failed to update skills usage keys %s -> %s: %s", old_username, new_username, err) + except Exception as e: + logger.warning("Failed to rename skills owner references %s -> %s: %s", old_username, new_username, e) + + # The in-memory session cache (session_manager.sessions) stores each + # session's owner at load time. Without this patch the renamed user's + # sessions are invisible on the next /api/sessions call because + # get_sessions_for_user does an exact `s.owner == username` comparison + # against stale in-memory values. + sm = getattr(request.app.state, "session_manager", None) + if sm is not None: + for sess in list(getattr(sm, "sessions", {}).values()): + if str(getattr(sess, "owner", None) or "").strip().lower() == old_username: + sess.owner = new_username + # The owner-rename loop above updated ApiToken.owner in the DB, but the # bearer-token cache still maps each token to the OLD owner. Without # refreshing it, the renamed user's API tokens resolve to the old (now diff --git a/tests/test_rename_user_owner_sync.py b/tests/test_rename_user_owner_sync.py new file mode 100644 index 000000000..16d91c512 --- /dev/null +++ b/tests/test_rename_user_owner_sync.py @@ -0,0 +1,384 @@ +"""Renaming a user must update all three owner caches, not just the SQL DB. + +The DB owner-rename loop in the rename_user route updates every SQL-backed +owner column, but three file-backed / in-memory stores are left stale: + +1. session_manager.sessions — in-memory session objects carry s.owner set at + load time; get_sessions_for_user does an exact `s.owner == username` check, + so the renamed user's sidebar empties until a server restart. + +2. data/deep_research/*.json — each report JSON has an `owner` field; + research_routes filters by `d.get("owner") == user`, making every report + invisible after rename. + +3. data/memory.json — a flat array where every entry has an `owner` field; + memory_manager.load(owner=user) filters on it, so all memories vanish. + +Regression coverage: these bugs are invisible in unit tests that mock the DB +loop but don't exercise the file/cache patches added to the route. +""" +import asyncio +import json +import sys +import types +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + + +def _route(router, name): + for r in router.routes: + if getattr(getattr(r, "endpoint", None), "__name__", "") == name: + return r.endpoint + raise AssertionError(name) + + +@pytest.fixture +def rename_endpoint(monkeypatch, tmp_path): + import routes.auth_routes as ar + import core.database as cdb + + # Neutralize the DB owner-rename loop. + monkeypatch.setattr(cdb, "SessionLocal", lambda: MagicMock()) + monkeypatch.setattr(cdb, "Base", SimpleNamespace(registry=SimpleNamespace(mappers=[])), raising=False) + # Neutralize the JSON-prefs rename. + pr = types.ModuleType("routes.prefs_routes") + pr._load = lambda: {} + pr._save = lambda d: None + monkeypatch.setitem(sys.modules, "routes.prefs_routes", pr) + # Patch the module-level constants so file-update steps write to tmp_path. + # (Patching sc.DATA_DIR wouldn't work — auth_routes binds DEEP_RESEARCH_DIR + # and MEMORY_FILE at import time, so we must patch those names on the module.) + monkeypatch.setattr(ar, "DEEP_RESEARCH_DIR", str(tmp_path / "deep_research")) + monkeypatch.setattr(ar, "MEMORY_FILE", str(tmp_path / "memory.json")) + monkeypatch.setattr(ar, "SKILLS_DIR", str(tmp_path / "skills")) + + am = MagicMock() + am.is_admin.return_value = True + am.get_username_for_token.return_value = "admin" + am.users = {"alice": {}} + am.rename_user.return_value = True + return _route(ar.setup_auth_routes(am), "rename_user"), am, tmp_path + + +def _request(tmp_path, session_manager=None): + state = SimpleNamespace( + invalidate_token_cache=lambda: None, + session_manager=session_manager, + ) + return SimpleNamespace( + cookies={"odysseus_session": "t"}, + app=SimpleNamespace(state=state), + state=SimpleNamespace(current_user="admin"), + ) + + +# --------------------------------------------------------------------------- +# 1. In-memory session cache +# --------------------------------------------------------------------------- + +def test_rename_updates_in_memory_session_owner(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + # Build a fake session_manager with one session owned by alice. + sess = SimpleNamespace(owner="alice") + sm = SimpleNamespace(sessions={"s1": sess}) + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path, sm))) + + assert sess.owner == "alice2", "in-memory session owner was not updated on rename" + + +def test_rename_session_owner_case_insensitive(rename_endpoint): + """Stored owner 'Alice' (mixed case) must match rename of 'alice'.""" + endpoint, _am, tmp_path = rename_endpoint + + sess = SimpleNamespace(owner="Alice") + sm = SimpleNamespace(sessions={"s1": sess}) + + asyncio.run(endpoint("alice", SimpleNamespace(username="bob"), _request(tmp_path, sm))) + + assert sess.owner == "bob" + + +def test_rename_leaves_other_sessions_untouched(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + sess_alice = SimpleNamespace(owner="alice") + sess_other = SimpleNamespace(owner="carol") + sm = SimpleNamespace(sessions={"s1": sess_alice, "s2": sess_other}) + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path, sm))) + + assert sess_alice.owner == "alice2" + assert sess_other.owner == "carol", "unrelated session owner was modified" + + +def test_rename_no_session_manager_does_not_crash(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + # app.state without a session_manager must not raise. + req = SimpleNamespace( + cookies={"odysseus_session": "t"}, + app=SimpleNamespace(state=SimpleNamespace(invalidate_token_cache=lambda: None)), + state=SimpleNamespace(current_user="admin"), + ) + res = asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), req)) + assert res["ok"] is True + + +# --------------------------------------------------------------------------- +# 2. deep_research JSON files +# --------------------------------------------------------------------------- + +def test_rename_updates_research_json_owner(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + dr_dir = tmp_path / "deep_research" + dr_dir.mkdir() + report = {"query": "test", "owner": "alice", "status": "done"} + p = dr_dir / "abc123.json" + p.write_text(json.dumps(report), encoding="utf-8") + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + + updated = json.loads(p.read_text(encoding="utf-8")) + assert updated["owner"] == "alice2", "deep_research JSON owner was not updated on rename" + + +def test_rename_research_json_case_insensitive(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + dr_dir = tmp_path / "deep_research" + dr_dir.mkdir() + p = (dr_dir / "r1.json") + p.write_text(json.dumps({"owner": "Alice"}), encoding="utf-8") + + asyncio.run(endpoint("alice", SimpleNamespace(username="bob"), _request(tmp_path))) + + assert json.loads(p.read_text())["owner"] == "bob" + + +def test_rename_leaves_other_research_untouched(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + dr_dir = tmp_path / "deep_research" + dr_dir.mkdir() + p_alice = dr_dir / "a.json" + p_carol = dr_dir / "c.json" + p_alice.write_text(json.dumps({"owner": "alice"}), encoding="utf-8") + p_carol.write_text(json.dumps({"owner": "carol"}), encoding="utf-8") + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + + assert json.loads(p_alice.read_text())["owner"] == "alice2" + assert json.loads(p_carol.read_text())["owner"] == "carol" + + +def test_rename_no_deep_research_dir_does_not_crash(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + # No deep_research dir — must not crash. + res = asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + assert res["ok"] is True + + +def test_rename_research_respects_custom_data_dir(monkeypatch, tmp_path): + """DEEP_RESEARCH_DIR (which honours ODYSSEUS_DATA_DIR) is used, not a + hardcoded relative path. Before the fix, setting ODYSSEUS_DATA_DIR made + the rename silently patch a different directory from where research files + actually live, so reports still disappeared after rename.""" + import routes.auth_routes as ar + import core.database as cdb + + custom_dr = tmp_path / "custom_data" / "deep_research" + custom_dr.mkdir(parents=True) + p = custom_dr / "rp-abc.json" + p.write_text(json.dumps({"query": "q", "owner": "alice", "status": "done"}), encoding="utf-8") + + monkeypatch.setattr(cdb, "SessionLocal", lambda: MagicMock()) + monkeypatch.setattr(cdb, "Base", SimpleNamespace(registry=SimpleNamespace(mappers=[])), raising=False) + pr = types.ModuleType("routes.prefs_routes") + pr._load = lambda: {} + pr._save = lambda d: None + monkeypatch.setitem(sys.modules, "routes.prefs_routes", pr) + monkeypatch.setattr(ar, "DEEP_RESEARCH_DIR", str(custom_dr)) + monkeypatch.setattr(ar, "MEMORY_FILE", str(tmp_path / "memory.json")) + + am = MagicMock() + am.is_admin.return_value = True + am.get_username_for_token.return_value = "admin" + am.users = {"alice": {}} + am.rename_user.return_value = True + endpoint = _route(ar.setup_auth_routes(am), "rename_user") + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + + assert json.loads(p.read_text(encoding="utf-8"))["owner"] == "alice2", ( + "research JSON at custom DATA_DIR was not patched — DEEP_RESEARCH_DIR constant not used" + ) + + +# --------------------------------------------------------------------------- +# 3. memory.json +# --------------------------------------------------------------------------- + +def test_rename_updates_memory_json_owner(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + entries = [ + {"id": "1", "text": "Lives in Berlin", "owner": "alice"}, + {"id": "2", "text": "Likes Python", "owner": "carol"}, + ] + (tmp_path / "memory.json").write_text(json.dumps(entries), encoding="utf-8") + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + + updated = json.loads((tmp_path / "memory.json").read_text(encoding="utf-8")) + assert updated[0]["owner"] == "alice2", "memory.json entry owner was not updated on rename" + assert updated[1]["owner"] == "carol", "unrelated memory entry was modified" + + +def test_rename_memory_json_case_insensitive(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + entries = [{"id": "1", "text": "x", "owner": "Alice"}] + (tmp_path / "memory.json").write_text(json.dumps(entries), encoding="utf-8") + + asyncio.run(endpoint("alice", SimpleNamespace(username="bob"), _request(tmp_path))) + + assert json.loads((tmp_path / "memory.json").read_text())[0]["owner"] == "bob" + + +def test_rename_no_memory_json_does_not_crash(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + # No memory.json — must not crash. + res = asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + assert res["ok"] is True + + +# --------------------------------------------------------------------------- +# 4. Skills (SKILL.md frontmatter + _usage.json sidecar) +# --------------------------------------------------------------------------- + +_SKILL_MD = """\ +--- +name: test-skill +description: A test skill. +version: 1.0.0 +category: general +status: published +confidence: 0.9 +source: learned +owner: {owner} +--- + +## When to Use +When testing. +""" + + +def test_rename_updates_skill_md_owner(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + skill_dir = tmp_path / "skills" / "general" / "test-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text(_SKILL_MD.format(owner="alice"), encoding="utf-8") + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + + content = (skill_dir / "SKILL.md").read_text(encoding="utf-8") + assert "owner: alice2" in content + assert "owner: alice\n" not in content + + +def test_rename_leaves_other_skill_owners_untouched(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + for owner, name in [("alice", "alice-skill"), ("carol", "carol-skill")]: + d = tmp_path / "skills" / "general" / name + d.mkdir(parents=True) + (d / "SKILL.md").write_text(_SKILL_MD.format(owner=owner).replace("test-skill", name), encoding="utf-8") + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + + assert "owner: alice2" in (tmp_path / "skills" / "general" / "alice-skill" / "SKILL.md").read_text() + assert "owner: carol" in (tmp_path / "skills" / "general" / "carol-skill" / "SKILL.md").read_text() + + +def test_rename_updates_usage_sidecar_keys(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + + skills_root = tmp_path / "skills" + skills_root.mkdir(parents=True) + usage = { + "alice::test-skill": {"uses": 3, "last_used": 1000}, + "carol::other-skill": {"uses": 1, "last_used": 500}, + "unscoped-skill": {"uses": 2, "last_used": 200}, + } + (skills_root / "_usage.json").write_text(json.dumps(usage), encoding="utf-8") + + asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + + updated = json.loads((skills_root / "_usage.json").read_text(encoding="utf-8")) + assert "alice2::test-skill" in updated + assert "alice::test-skill" not in updated + assert "carol::other-skill" in updated + assert "unscoped-skill" in updated + + +def test_rename_no_skills_dir_does_not_crash(rename_endpoint): + endpoint, _am, tmp_path = rename_endpoint + res = asyncio.run(endpoint("alice", SimpleNamespace(username="alice2"), _request(tmp_path))) + assert res["ok"] is True + + +# --------------------------------------------------------------------------- +# 5. P1 regression: rejected auth rename must not mutate file-backed stores +# --------------------------------------------------------------------------- + +def test_rejected_rename_does_not_mutate_files(monkeypatch, tmp_path): + """If auth_manager.rename_user() returns False, no file-backed store + should be touched. Before the fix the deep_research and memory writes + ran before the auth check, so a rejected rename (e.g. reserved username) + silently moved owner fields to the new name.""" + import routes.auth_routes as ar + import core.database as cdb + + monkeypatch.setattr(cdb, "SessionLocal", lambda: MagicMock()) + monkeypatch.setattr(cdb, "Base", SimpleNamespace(registry=SimpleNamespace(mappers=[])), raising=False) + pr = types.ModuleType("routes.prefs_routes") + pr._load = lambda: {} + pr._save = lambda d: None + monkeypatch.setitem(sys.modules, "routes.prefs_routes", pr) + monkeypatch.setattr(ar, "DEEP_RESEARCH_DIR", str(tmp_path / "deep_research")) + monkeypatch.setattr(ar, "MEMORY_FILE", str(tmp_path / "memory.json")) + monkeypatch.setattr(ar, "SKILLS_DIR", str(tmp_path / "skills")) + + # Seed files for alice. + dr = tmp_path / "deep_research" + dr.mkdir() + rp = dr / "rp-abc.json" + rp.write_text(json.dumps({"owner": "alice", "query": "q"}), encoding="utf-8") + + mem = tmp_path / "memory.json" + mem.write_text(json.dumps([{"owner": "alice", "text": "x"}]), encoding="utf-8") + + skill_dir = tmp_path / "skills" / "general" / "s" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text(_SKILL_MD.format(owner="alice"), encoding="utf-8") + + # Auth rejects the rename (reserved name, race, etc.). + am = MagicMock() + am.is_admin.return_value = True + am.get_username_for_token.return_value = "admin" + am.users = {"alice": {}} + am.rename_user.return_value = False + endpoint = _route(ar.setup_auth_routes(am), "rename_user") + + with pytest.raises(Exception): + asyncio.run(endpoint("alice", SimpleNamespace(username="api"), _request(tmp_path))) + + assert json.loads(rp.read_text())["owner"] == "alice", "research owner mutated after rejected rename" + assert json.loads(mem.read_text())[0]["owner"] == "alice", "memory owner mutated after rejected rename" + assert "owner: alice" in (skill_dir / "SKILL.md").read_text(), "skill owner mutated after rejected rename"