feat(persistence/authz): PR6 T6.4 — check_access takes workspace_id
`ThreadMetaStore.check_access(thread_id, user_id, workspace_id, *, require_existing)` is now a three-positional method. Cross-workspace is denied unconditionally — even when the row exists and `user_id` matches — so the decorator layer can convert the False into a **404** and never leak the existence of a thread across tenants. Inside the workspace, the existing legacy semantics still hold (NULL `row.user_id` stays "shared in workspace", `require_existing` still gates the missing-row path against ghost-row re-targeting). `@require_permission(owner_check=True)` in `app/gateway/authz.py` now reads the active workspace from `get_effective_workspace_id()` (set by PR4 AuthMiddleware; falls back to "default" in no-auth dev) and passes it through. The existing 404-not-403 mapping is unchanged. Existing positional callers in `test_thread_meta_repo.py` and the permissive mock in `test_threads_router.py` were updated for the new arity. 58 thread_meta / router / memory tests stay green; 90 auth / uploads / suggestions tests stay green.
This commit is contained in:
@@ -268,24 +268,27 @@ def require_permission(
|
||||
|
||||
# Owner check for thread-specific resources.
|
||||
#
|
||||
# 2.0-rc moved thread metadata into the SQL persistence layer
|
||||
# (``threads_meta`` table). We verify ownership via
|
||||
# ``ThreadMetaStore.check_access``: it returns True for
|
||||
# missing rows (untracked legacy thread) and for rows whose
|
||||
# ``user_id`` is NULL (shared / pre-auth data), so this is
|
||||
# strict-deny rather than strict-allow — only an *existing*
|
||||
# row with a *different* user_id triggers 404.
|
||||
# PR6: ``check_access`` now takes ``workspace_id`` as the third
|
||||
# positional argument; cross-workspace always denies regardless
|
||||
# of user_id match. We pull workspace_id from the contextvar
|
||||
# AuthMiddleware sets per request (and fall back to "default"
|
||||
# in no-auth dev mode so smoke flows keep working). Failures
|
||||
# convert to **404**, not 403, so the response never leaks the
|
||||
# existence of a thread that belongs to a different tenant.
|
||||
if owner_check:
|
||||
thread_id = kwargs.get("thread_id")
|
||||
if thread_id is None:
|
||||
raise ValueError("require_permission with owner_check=True requires 'thread_id' parameter")
|
||||
|
||||
from app.gateway.deps import get_thread_store
|
||||
from deerflow.runtime.workspace_context import get_effective_workspace_id
|
||||
|
||||
workspace_id = get_effective_workspace_id()
|
||||
thread_store = get_thread_store(request)
|
||||
allowed = await thread_store.check_access(
|
||||
thread_id,
|
||||
str(auth.user.id),
|
||||
workspace_id,
|
||||
require_existing=require_existing,
|
||||
)
|
||||
if not allowed:
|
||||
|
||||
@@ -116,10 +116,20 @@ class MemoryThreadMetaStore(ThreadMetaStore):
|
||||
)
|
||||
return [self._item_to_dict(item) for item in items]
|
||||
|
||||
async def check_access(self, thread_id: str, user_id: str, *, require_existing: bool = False) -> bool:
|
||||
async def check_access(
|
||||
self,
|
||||
thread_id: str,
|
||||
user_id: str,
|
||||
workspace_id: str,
|
||||
*,
|
||||
require_existing: bool = False,
|
||||
) -> bool:
|
||||
item = await self._store.aget(THREADS_NS, thread_id)
|
||||
if item is None:
|
||||
return not require_existing
|
||||
record_workspace_id = item.value.get("workspace_id")
|
||||
if record_workspace_id is not None and record_workspace_id != workspace_id:
|
||||
return False
|
||||
record_user_id = item.value.get("user_id")
|
||||
if record_user_id is None:
|
||||
return True
|
||||
|
||||
@@ -87,32 +87,36 @@ class ThreadMetaRepository(ThreadMetaStore):
|
||||
return None
|
||||
return self._row_to_dict(row)
|
||||
|
||||
async def check_access(self, thread_id: str, user_id: str, *, require_existing: bool = False) -> bool:
|
||||
"""Check if ``user_id`` has access to ``thread_id``.
|
||||
async def check_access(
|
||||
self,
|
||||
thread_id: str,
|
||||
user_id: str,
|
||||
workspace_id: str,
|
||||
*,
|
||||
require_existing: bool = False,
|
||||
) -> bool:
|
||||
"""Check if ``user_id`` in ``workspace_id`` has access to ``thread_id``.
|
||||
|
||||
Two modes — one row, two distinct semantics depending on what
|
||||
the caller is about to do:
|
||||
Three filters layered, from outside in:
|
||||
|
||||
- ``require_existing=False`` (default, permissive):
|
||||
Returns True for: row missing (untracked legacy thread),
|
||||
``row.user_id`` is None (shared / pre-auth data),
|
||||
or ``row.user_id == user_id``. Use for **read-style**
|
||||
decorators where treating an untracked thread as accessible
|
||||
preserves backward-compat.
|
||||
|
||||
- ``require_existing=True`` (strict):
|
||||
Returns True **only** when the row exists AND
|
||||
(``row.user_id == user_id`` OR ``row.user_id is None``).
|
||||
Use for **destructive / mutating** decorators (DELETE, PATCH,
|
||||
state-update) so a thread that has *already been deleted*
|
||||
cannot be re-targeted by any caller — closing the
|
||||
delete-idempotence cross-user gap where the row vanishing
|
||||
made every other user appear to "own" it.
|
||||
- Cross-workspace is **always** denied (returns False), even when
|
||||
the row exists and ``user_id`` matches. The decorator layer
|
||||
converts a False into a 404 so cross-tenant access never leaks
|
||||
the existence of a thread.
|
||||
- Missing row honours ``require_existing``: False by default
|
||||
(permissive — untracked legacy threads still readable), True
|
||||
for destructive routes (DELETE / PATCH) so a re-targeted ghost
|
||||
row cannot be claimed.
|
||||
- Within the workspace, ``row.user_id IS NULL`` keeps the legacy
|
||||
"shared / pre-auth" semantics — readable by anyone in the
|
||||
workspace. ``row.user_id == user_id`` is the normal case.
|
||||
"""
|
||||
async with self._sf() as session:
|
||||
row = await session.get(ThreadMetaRow, thread_id)
|
||||
if row is None:
|
||||
return not require_existing
|
||||
if row.workspace_id is not None and row.workspace_id != workspace_id:
|
||||
return False
|
||||
if row.user_id is None:
|
||||
return True
|
||||
return row.user_id == user_id
|
||||
|
||||
@@ -64,21 +64,21 @@ class TestThreadMetaRepository:
|
||||
@pytest.mark.anyio
|
||||
async def test_check_access_no_record_allows(self, tmp_path):
|
||||
repo = await _make_repo(tmp_path)
|
||||
assert await repo.check_access("unknown", "user1") is True
|
||||
assert await repo.check_access("unknown", "user1", "test-workspace-autouse") is True
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_check_access_owner_matches(self, tmp_path):
|
||||
repo = await _make_repo(tmp_path)
|
||||
await repo.create("t1", user_id="user1")
|
||||
assert await repo.check_access("t1", "user1") is True
|
||||
assert await repo.check_access("t1", "user1", "test-workspace-autouse") is True
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_check_access_owner_mismatch(self, tmp_path):
|
||||
repo = await _make_repo(tmp_path)
|
||||
await repo.create("t1", user_id="user1")
|
||||
assert await repo.check_access("t1", "user2") is False
|
||||
assert await repo.check_access("t1", "user2", "test-workspace-autouse") is False
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
@@ -87,7 +87,7 @@ class TestThreadMetaRepository:
|
||||
# Explicit user_id=None to bypass the new AUTO default that
|
||||
# would otherwise pick up the test user from the autouse fixture.
|
||||
await repo.create("t1", user_id=None)
|
||||
assert await repo.check_access("t1", "anyone") is True
|
||||
assert await repo.check_access("t1", "anyone", "test-workspace-autouse") is True
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
@@ -99,21 +99,21 @@ class TestThreadMetaRepository:
|
||||
caller "claim" it as untracked. The strict mode demands a row.
|
||||
"""
|
||||
repo = await _make_repo(tmp_path)
|
||||
assert await repo.check_access("never-existed", "user1", require_existing=True) is False
|
||||
assert await repo.check_access("never-existed", "user1", "test-workspace-autouse", require_existing=True) is False
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_check_access_strict_owner_match_allowed(self, tmp_path):
|
||||
repo = await _make_repo(tmp_path)
|
||||
await repo.create("t1", user_id="user1")
|
||||
assert await repo.check_access("t1", "user1", require_existing=True) is True
|
||||
assert await repo.check_access("t1", "user1", "test-workspace-autouse", require_existing=True) is True
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_check_access_strict_owner_mismatch_denied(self, tmp_path):
|
||||
repo = await _make_repo(tmp_path)
|
||||
await repo.create("t1", user_id="user1")
|
||||
assert await repo.check_access("t1", "user2", require_existing=True) is False
|
||||
assert await repo.check_access("t1", "user2", "test-workspace-autouse", require_existing=True) is False
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
@@ -126,7 +126,7 @@ class TestThreadMetaRepository:
|
||||
"""
|
||||
repo = await _make_repo(tmp_path)
|
||||
await repo.create("t1", user_id=None)
|
||||
assert await repo.check_access("t1", "anyone", require_existing=True) is True
|
||||
assert await repo.check_access("t1", "anyone", "test-workspace-autouse", require_existing=True) is True
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
|
||||
@@ -237,6 +237,36 @@ class TestSearchUpdateDeleteWorkspace:
|
||||
await _cleanup()
|
||||
assert row["metadata"] == {"k": "alpha"}
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_check_access_cross_workspace_false(self, tmp_path):
|
||||
"""`check_access` returns False for cross-workspace, even with matching user_id."""
|
||||
repo = await _make_repo(tmp_path, workspaces=("ws-alpha", "ws-beta"))
|
||||
token = _use_workspace("ws-alpha")
|
||||
try:
|
||||
await repo.create("t1", user_id="alice")
|
||||
finally:
|
||||
reset_current_workspace(token)
|
||||
try:
|
||||
assert await repo.check_access("t1", "alice", "ws-beta") is False
|
||||
assert await repo.check_access("t1", "alice", "ws-alpha") is True
|
||||
finally:
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_check_access_strict_cross_workspace_false(self, tmp_path):
|
||||
"""require_existing=True path also denies cross-workspace."""
|
||||
repo = await _make_repo(tmp_path, workspaces=("ws-alpha", "ws-beta"))
|
||||
token = _use_workspace("ws-alpha")
|
||||
try:
|
||||
await repo.create("t1", user_id="alice")
|
||||
finally:
|
||||
reset_current_workspace(token)
|
||||
try:
|
||||
assert await repo.check_access("t1", "alice", "ws-beta", require_existing=True) is False
|
||||
assert await repo.check_access("t1", "alice", "ws-alpha", require_existing=True) is True
|
||||
finally:
|
||||
await _cleanup()
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_delete_blocked_across_workspace(self, tmp_path):
|
||||
repo = await _make_repo(tmp_path, workspaces=("ws-alpha", "ws-beta"))
|
||||
|
||||
@@ -27,21 +27,21 @@ class _PermissiveThreadMetaStore(MemoryThreadMetaStore):
|
||||
timestamp wire format.
|
||||
"""
|
||||
|
||||
async def _get_owned_record(self, thread_id, user_id, method_name): # type: ignore[override]
|
||||
async def _get_owned_record(self, thread_id, user_id, workspace_id, method_name): # type: ignore[override]
|
||||
item = await self._store.aget(THREADS_NS, thread_id)
|
||||
return dict(item.value) if item is not None else None
|
||||
|
||||
async def check_access(self, thread_id, user_id, *, require_existing=False): # type: ignore[override]
|
||||
async def check_access(self, thread_id, user_id, workspace_id, *, require_existing=False): # type: ignore[override]
|
||||
item = await self._store.aget(THREADS_NS, thread_id)
|
||||
if item is None:
|
||||
return not require_existing
|
||||
return True
|
||||
|
||||
async def create(self, thread_id, *, assistant_id=None, user_id=None, display_name=None, metadata=None): # type: ignore[override]
|
||||
return await super().create(thread_id, assistant_id=assistant_id, user_id=None, display_name=display_name, metadata=metadata)
|
||||
async def create(self, thread_id, *, assistant_id=None, user_id=None, workspace_id=None, display_name=None, metadata=None): # type: ignore[override]
|
||||
return await super().create(thread_id, assistant_id=assistant_id, user_id=None, workspace_id=None, display_name=display_name, metadata=metadata)
|
||||
|
||||
async def search(self, *, metadata=None, status=None, limit=100, offset=0, user_id=None): # type: ignore[override]
|
||||
return await super().search(metadata=metadata, status=status, limit=limit, offset=offset, user_id=None)
|
||||
async def search(self, *, metadata=None, status=None, limit=100, offset=0, user_id=None, workspace_id=None): # type: ignore[override]
|
||||
return await super().search(metadata=metadata, status=status, limit=limit, offset=offset, user_id=None, workspace_id=None)
|
||||
|
||||
|
||||
def _build_thread_app() -> tuple[FastAPI, InMemoryStore, InMemorySaver]:
|
||||
|
||||
Reference in New Issue
Block a user