diff --git a/backend/app/gateway/authz.py b/backend/app/gateway/authz.py index c7cf6385..0579f72e 100644 --- a/backend/app/gateway/authz.py +++ b/backend/app/gateway/authz.py @@ -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: diff --git a/backend/packages/harness/deerflow/persistence/thread_meta/memory.py b/backend/packages/harness/deerflow/persistence/thread_meta/memory.py index 9afd1a75..9d04985f 100644 --- a/backend/packages/harness/deerflow/persistence/thread_meta/memory.py +++ b/backend/packages/harness/deerflow/persistence/thread_meta/memory.py @@ -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 diff --git a/backend/packages/harness/deerflow/persistence/thread_meta/sql.py b/backend/packages/harness/deerflow/persistence/thread_meta/sql.py index bd845579..4cd6c85d 100644 --- a/backend/packages/harness/deerflow/persistence/thread_meta/sql.py +++ b/backend/packages/harness/deerflow/persistence/thread_meta/sql.py @@ -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 diff --git a/backend/tests/test_thread_meta_repo.py b/backend/tests/test_thread_meta_repo.py index 3a653256..113227bf 100644 --- a/backend/tests/test_thread_meta_repo.py +++ b/backend/tests/test_thread_meta_repo.py @@ -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 diff --git a/backend/tests/test_thread_meta_workspace_filter.py b/backend/tests/test_thread_meta_workspace_filter.py index 511540fe..f9bc1adc 100644 --- a/backend/tests/test_thread_meta_workspace_filter.py +++ b/backend/tests/test_thread_meta_workspace_filter.py @@ -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")) diff --git a/backend/tests/test_threads_router.py b/backend/tests/test_threads_router.py index daf0c0b1..c13fb1e6 100644 --- a/backend/tests/test_threads_router.py +++ b/backend/tests/test_threads_router.py @@ -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]: