A session that exists only in the in-memory SessionManager — never persisted, or whose DB row was removed out-of-band — was listed by GET /api/sessions (the list is built from the in-memory manager) but 404'd on every per-session operation, so it could never be deleted. Two causes, both fixed: 1. _verify_session_owner() only consulted the DB and raised 404 when no row existed. It now falls back to the in-memory session's owner when (and only when) a session_manager is supplied and the caller actually owns the ghost. The DB row stays authoritative when present, and a ghost owned by another user still 404s, so the ownership/security model is unchanged. The new parameter defaults to None, preserving behavior for all other callers. 2. SessionManager.delete_session() only removed the in-memory entry when a DB row was found, so memory-only ghosts survived. It now drops the in-memory copy regardless and reports success when either the DB row or the in-memory entry was removed. Added tests/test_session_ghost_delete.py covering both layers, including the cross-owner 404, the unauthenticated 403, DB-row-wins precedence, and backward compatibility when no manager is passed. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
136 lines
5.9 KiB
Python
136 lines
5.9 KiB
Python
"""Regression tests for issue #1044 — "ghost" sessions that appear in the list
|
|
but 404 on every operation and can never be deleted.
|
|
|
|
A ghost session lives only in the in-memory ``SessionManager`` (it was never
|
|
persisted, or its DB row was removed out-of-band). ``GET /api/sessions`` lists
|
|
sessions from the in-memory manager, so a ghost shows up; but ``_verify_session_owner``
|
|
only consulted the DB, so every per-session op 404'd, and ``SessionManager.delete_session``
|
|
only dropped the in-memory copy when a DB row existed — so the ghost was undeletable.
|
|
|
|
These tests pin both halves of the fix while proving the ownership/security model
|
|
is preserved (a ghost owned by another user still 404s; the DB row stays
|
|
authoritative when present).
|
|
|
|
Style mirrors tests/test_session_owner_attribution.py: stub the heavy ORM modules
|
|
so the real route + manager code can be imported under the MagicMock sqlalchemy
|
|
stub from conftest.
|
|
"""
|
|
|
|
import sys
|
|
import importlib
|
|
from types import SimpleNamespace
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
# Import the *real* core.session_manager + routes.session_routes under conftest's
|
|
# MagicMock sqlalchemy stub. The real core.database defines declarative classes
|
|
# that blow up under that stub, so temporarily swap in MagicMock module objects
|
|
# (auto-creating attributes satisfy any `from core.database import X`). Crucially
|
|
# we RESTORE sys.modules immediately after import so these stubs never leak into
|
|
# sibling test modules — the imported SM/SR objects keep their captured bindings.
|
|
_ABSENT = object()
|
|
_TEMP_STUBS = ("core.database", "core.models", "src.request_models")
|
|
_saved = {name: sys.modules.get(name, _ABSENT) for name in _TEMP_STUBS}
|
|
_saved["core.session_manager"] = sys.modules.get("core.session_manager", _ABSENT)
|
|
try:
|
|
for _name in _TEMP_STUBS:
|
|
sys.modules[_name] = MagicMock(name=_name)
|
|
if isinstance(sys.modules.get("core.session_manager"), MagicMock):
|
|
del sys.modules["core.session_manager"]
|
|
SM = importlib.import_module("core.session_manager")
|
|
import routes.session_routes as SR # noqa: E402
|
|
finally:
|
|
for _name, _val in _saved.items():
|
|
if _val is _ABSENT:
|
|
sys.modules.pop(_name, None)
|
|
else:
|
|
sys.modules[_name] = _val
|
|
|
|
from fastapi import HTTPException # noqa: E402
|
|
|
|
|
|
_MISSING = object()
|
|
|
|
|
|
def _req(**state):
|
|
return SimpleNamespace(state=SimpleNamespace(**state))
|
|
|
|
|
|
def _session_local_returning(owner_value):
|
|
"""Mock SessionLocal whose query(...).filter(...).first() yields a row with
|
|
the given owner, or None when owner_value is _MISSING ('no DB row')."""
|
|
db = MagicMock()
|
|
row = None if owner_value is _MISSING else SimpleNamespace(owner=owner_value)
|
|
db.query.return_value.filter.return_value.first.return_value = row
|
|
return MagicMock(return_value=db)
|
|
|
|
|
|
def _manager_with(sessions):
|
|
"""A SessionManager instance with the given in-memory sessions and no __init__."""
|
|
mgr = SM.SessionManager.__new__(SM.SessionManager)
|
|
mgr.sessions = dict(sessions)
|
|
return mgr
|
|
|
|
|
|
# --- route layer: _verify_session_owner ghost fallback ---------------------
|
|
|
|
def test_owned_ghost_is_allowed_when_manager_passed(monkeypatch):
|
|
# No DB row, but the caller owns the in-memory ghost -> must NOT raise.
|
|
monkeypatch.setattr(SR, "SessionLocal", _session_local_returning(_MISSING))
|
|
sm = SimpleNamespace(sessions={"ghost": SimpleNamespace(owner="alice")})
|
|
SR._verify_session_owner(_req(api_token=False, current_user="alice"), "ghost", sm)
|
|
|
|
|
|
def test_ghost_owned_by_another_user_still_404(monkeypatch):
|
|
# Security: a ghost owned by bob must never be reachable by alice.
|
|
monkeypatch.setattr(SR, "SessionLocal", _session_local_returning(_MISSING))
|
|
sm = SimpleNamespace(sessions={"ghost": SimpleNamespace(owner="bob")})
|
|
with pytest.raises(HTTPException) as exc:
|
|
SR._verify_session_owner(_req(api_token=False, current_user="alice"), "ghost", sm)
|
|
assert exc.value.status_code == 404
|
|
|
|
|
|
def test_no_manager_keeps_legacy_404(monkeypatch):
|
|
# Backward compat: callers that don't pass a manager behave exactly as before.
|
|
monkeypatch.setattr(SR, "SessionLocal", _session_local_returning(_MISSING))
|
|
with pytest.raises(HTTPException) as exc:
|
|
SR._verify_session_owner(_req(api_token=False, current_user="alice"), "ghost")
|
|
assert exc.value.status_code == 404
|
|
|
|
|
|
def test_db_row_stays_authoritative(monkeypatch):
|
|
# When a DB row exists it wins; the ghost map is not consulted.
|
|
monkeypatch.setattr(SR, "SessionLocal", _session_local_returning("alice"))
|
|
sm = SimpleNamespace(sessions={"sid": SimpleNamespace(owner="bob")})
|
|
SR._verify_session_owner(_req(api_token=False, current_user="alice"), "sid", sm)
|
|
|
|
|
|
def test_unauthenticated_still_403(monkeypatch):
|
|
monkeypatch.setattr(SR, "SessionLocal", _session_local_returning(_MISSING))
|
|
sm = SimpleNamespace(sessions={"ghost": SimpleNamespace(owner=None)})
|
|
with pytest.raises(HTTPException) as exc:
|
|
SR._verify_session_owner(_req(api_token=False, current_user=None), "ghost", sm)
|
|
assert exc.value.status_code == 403
|
|
|
|
|
|
# --- manager layer: delete_session clears memory-only ghosts ---------------
|
|
|
|
def test_manager_deletes_memory_only_ghost(monkeypatch):
|
|
# No DB row, but the session is in memory -> delete it and report success.
|
|
fake_db = MagicMock()
|
|
fake_db.query.return_value.filter.return_value.first.return_value = None
|
|
monkeypatch.setattr(SM, "SessionLocal", MagicMock(return_value=fake_db))
|
|
mgr = _manager_with({"ghost": SimpleNamespace(id="ghost", owner="alice")})
|
|
assert mgr.delete_session("ghost") is True
|
|
assert "ghost" not in mgr.sessions
|
|
|
|
|
|
def test_manager_delete_unknown_returns_false(monkeypatch):
|
|
# Nothing in the DB and nothing in memory -> nothing deleted.
|
|
fake_db = MagicMock()
|
|
fake_db.query.return_value.filter.return_value.first.return_value = None
|
|
monkeypatch.setattr(SM, "SessionLocal", MagicMock(return_value=fake_db))
|
|
mgr = _manager_with({})
|
|
assert mgr.delete_session("nope") is False
|