From c07434a22105c28c3480daf2372898bd444b7399 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Thu, 18 Jun 2026 09:10:09 -0700 Subject: [PATCH] fix: allow read-only cross-user access to agent threads via Open in Web links (#1568) The thread detail endpoint already returned metadata for non-owners, but the transcript hydration endpoints (state, stream/events, history, pr-diff) all asserted ownership and 404-ed. This caused the UI to redirect non-owners back to /agents when they clicked an "Open in Web" link shared in Slack. Dashboard login is already gated by ALLOWED_GITHUB_ORGS, so any logged-in user is a trusted org member. This commit: - Adds _thread_is_readable / _assert_thread_readable helpers that grant read access to any surfaced-source thread for authenticated users - Relaxes read endpoints (state, stream/events, history, pr-diff, SSE stream) to use readable checks instead of ownership checks - Keeps write endpoints (send message, cancel, delete, resolve, run commands) owner-only - Adds an isOwner field to the thread summary so the frontend can render a read-only mode (hides the prompt bar, resolve/delete buttons) Co-authored-by: open-swe[bot] --- agent/dashboard/thread_api.py | 59 ++++++++- tests/test_dashboard_thread_api.py | 125 +++++++++++++++++-- ui/src/components/agents/AgentThreadView.tsx | 69 +++++----- ui/src/components/agents/AgentsSidebar.tsx | 95 +++++++------- ui/src/lib/agents/queries.ts | 1 + ui/src/lib/agents/types.ts | 1 + 6 files changed, 261 insertions(+), 89 deletions(-) diff --git a/agent/dashboard/thread_api.py b/agent/dashboard/thread_api.py index bfaebdbe..d8945703 100644 --- a/agent/dashboard/thread_api.py +++ b/agent/dashboard/thread_api.py @@ -247,6 +247,22 @@ def _assert_thread_owner(metadata: dict[str, Any], login: str, email: str | None raise HTTPException(404, "thread not found") +def _thread_is_readable(metadata: dict[str, Any]) -> bool: + """Any surfaced-source thread is readable by authenticated users. + + Dashboard login is already gated by ``ALLOWED_GITHUB_ORGS`` (see + ``oauth.enforce_org_login_gate``), so any logged-in user is a trusted + org member. This lets teammates open "Open in Web" links shared in Slack + threads with read-only access. + """ + return _thread_source(metadata) in _SURFACED_SOURCES + + +def _assert_thread_readable(metadata: dict[str, Any]) -> None: + if not _thread_is_readable(metadata): + raise HTTPException(404, "thread not found") + + def _metadata_repo(metadata: dict[str, Any]) -> tuple[str, str, str]: owner = metadata.get("repo_owner") name = metadata.get("repo_name") @@ -296,6 +312,8 @@ def _thread_summary( *, latest_run_status: str | None = None, latest_run_id: str | None = None, + owner_login: str | None = None, + owner_email: str | None = None, ) -> dict[str, Any]: metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} owner, name, full_name = _metadata_repo(metadata) @@ -343,6 +361,7 @@ def _thread_summary( ), "createdAt": int(created_at) if isinstance(created_at, (int, float)) else _now_ms(), "updatedAt": int(updated_at) if isinstance(updated_at, (int, float)) else _now_ms(), + "isOwner": (_user_owns_thread(metadata, owner_login, owner_email) if owner_login else True), "traceUrl": trace_url, } if isinstance(pr_number, int) and isinstance(pr_url, str): @@ -662,6 +681,7 @@ async def get_dashboard_thread( raise HTTPException(404, "thread not found") from exc metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} + _assert_thread_readable(metadata) is_owner = _user_owns_thread(metadata, login, email) # The transcript is hydrated client-side by the SDK (`StreamProvider` reads @@ -691,6 +711,8 @@ async def get_dashboard_thread( thread, latest_run_status=latest_run_status, latest_run_id=latest_run_id, + owner_login=login, + owner_email=email, ) @@ -1096,10 +1118,35 @@ async def _authorized_thread( return thread +async def _readable_thread( + thread_id: str, *, login: str | None = None, email: str | None = None +) -> dict[str, Any]: + """Fetch a thread and assert it is readable by the requesting user. + + Read access is granted to any authenticated org member for surfaced-source + threads; ``login``/``email`` are accepted for API parity but not required. + """ + try: + thread = await langgraph_client().threads.get(thread_id) + except Exception as exc: # noqa: BLE001 + raise HTTPException(404, "thread not found") from exc + metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} + _assert_thread_readable(metadata) + return thread + + +async def _readable_thread_metadata( + thread_id: str, *, login: str | None = None, email: str | None = None +) -> dict[str, Any]: + thread = await _readable_thread(thread_id, login=login, email=email) + metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} + return metadata + + async def get_dashboard_thread_state( thread_id: str, login: str, *, email: str | None = None ) -> dict[str, Any]: - thread = await _authorized_thread(thread_id, login, email=email) + thread = await _readable_thread(thread_id, login=login, email=email) metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} state = await langgraph_client().threads.get_state(thread_id) result = state if isinstance(state, dict) else dict(state) @@ -1127,7 +1174,7 @@ async def _github_token_for_login(login: str) -> str: async def get_dashboard_thread_pr_diff( thread_id: str, login: str, *, email: str | None = None ) -> dict[str, Any]: - metadata = await _authorized_thread_metadata(thread_id, login, email=email) + metadata = await _readable_thread_metadata(thread_id, login=login, email=email) pr_number = metadata.get("pr_number") _, _, full_name = _metadata_repo(metadata) if not isinstance(pr_number, int) or not full_name: @@ -1162,7 +1209,7 @@ async def proxy_dashboard_thread_stream_events( # Preflight here (not in the generator) so auth/content-type failures # surface as real HTTP errors before the SSE response starts streaming. _require_json_content_type(content_type) - await _authorized_thread_metadata(thread_id, login, email=email) + await _readable_thread_metadata(thread_id, login=login, email=email) return _stream_thread_events(thread_id, body, content_type) @@ -1281,7 +1328,7 @@ async def proxy_dashboard_thread_history( content_type: str = "application/json", ) -> tuple[int, bytes, str | None]: _require_json_content_type(content_type) - await _authorized_thread_metadata(thread_id, login, email=email) + await _readable_thread_metadata(thread_id, login=login, email=email) url = f"{langgraph_url().rstrip('/')}/threads/{thread_id}/history" headers = _langgraph_proxy_headers(content_type=content_type) async with httpx.AsyncClient(timeout=_PROXY_REQUEST_TIMEOUT) as client: @@ -1331,9 +1378,11 @@ async def stream_dashboard_thread( thread_id: str, login: str, *, email: str | None = None, last_event_id: str | None = None ) -> AsyncIterator[str]: try: - await langgraph_client().threads.get(thread_id) + thread = await langgraph_client().threads.get(thread_id) except Exception as exc: # noqa: BLE001 raise HTTPException(404, "thread not found") from exc + metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} + _assert_thread_readable(metadata) stream = await langgraph_client().threads.join_stream( thread_id, diff --git a/tests/test_dashboard_thread_api.py b/tests/test_dashboard_thread_api.py index a71ce034..a16b8f3e 100644 --- a/tests/test_dashboard_thread_api.py +++ b/tests/test_dashboard_thread_api.py @@ -432,6 +432,8 @@ async def test_proxy_commands_rejects_non_object_body(monkeypatch) -> None: async def test_proxy_endpoints_enforce_thread_ownership(monkeypatch) -> None: + """Write endpoints (commands, run_cancel) still require thread ownership.""" + class FakeThreads: async def get(self, thread_id: str) -> dict[str, object]: assert thread_id == "tid" @@ -445,24 +447,86 @@ async def test_proxy_endpoints_enforce_thread_ownership(monkeypatch) -> None: monkeypatch.setattr(thread_api, "langgraph_client", lambda: FakeClient()) - with pytest.raises(HTTPException) as exc_info: - await thread_api.get_dashboard_thread_state("tid", "intruder") - assert exc_info.value.status_code == 404 - with pytest.raises(HTTPException) as exc_info: await thread_api.proxy_dashboard_thread_commands("tid", "intruder", b"{}") assert exc_info.value.status_code == 404 - with pytest.raises(HTTPException) as exc_info: - await thread_api.proxy_dashboard_thread_history("tid", "intruder", b"{}") - assert exc_info.value.status_code == 404 - with pytest.raises(HTTPException) as exc_info: await thread_api.proxy_dashboard_thread_run_cancel("tid", "run-1", "intruder") assert exc_info.value.status_code == 404 + +async def test_read_endpoints_accessible_by_non_owner(monkeypatch) -> None: + """Read endpoints (state, stream, history) are accessible by any org member.""" + + class FakeThreads: + async def get(self, thread_id: str) -> dict[str, object]: + assert thread_id == "tid" + return { + "thread_id": "tid", + "metadata": {"source": "slack", "github_login": "owner"}, + } + + async def get_state(self, thread_id: str) -> dict[str, object]: + return {"values": {"messages": []}} + + class FakeClient: + threads = FakeThreads() + + monkeypatch.setattr(thread_api, "langgraph_client", lambda: FakeClient()) + + # Read endpoints succeed for non-owners (org members). + state = await thread_api.get_dashboard_thread_state("tid", "teammate") + assert "values" in state + + # stream/events preflight should not raise. + await thread_api.proxy_dashboard_thread_stream_events( + "tid", "teammate", b"{}", content_type="application/json" + ) + + # history preflight should not raise; mock the proxied HTTP call. + class FakeResponse: + status_code = 200 + content = b"{}" + headers = {"content-type": "application/json"} + + class FakeAsyncClient: + def __init__(self, *a: object, **kw: object) -> None: + pass + + async def __aenter__(self) -> "FakeAsyncClient": + return self + + async def __aexit__(self, *a: object) -> None: + pass + + async def post(self, *a: object, **kw: object) -> FakeResponse: + return FakeResponse() + + monkeypatch.setattr(thread_api.httpx, "AsyncClient", FakeAsyncClient) + await thread_api.proxy_dashboard_thread_history("tid", "teammate", b"{}") + + +async def test_read_endpoints_reject_non_surfaced_source(monkeypatch) -> None: + """Threads with an unknown source are not readable by anyone.""" + + class FakeThreads: + async def get(self, thread_id: str) -> dict[str, object]: + return { + "thread_id": "tid", + "metadata": {"source": "unknown-source", "github_login": "owner"}, + } + + async def get_state(self, thread_id: str) -> dict[str, object]: + return {"values": {"messages": []}} + + class FakeClient: + threads = FakeThreads() + + monkeypatch.setattr(thread_api, "langgraph_client", lambda: FakeClient()) + with pytest.raises(HTTPException) as exc_info: - await thread_api.proxy_dashboard_thread_stream_events("tid", "intruder", b"{}") + await thread_api.get_dashboard_thread_state("tid", "owner") assert exc_info.value.status_code == 404 @@ -521,6 +585,49 @@ def test_thread_summary_defaults_to_not_resolved() -> None: assert summary["resolvedAt"] is None +def test_thread_summary_is_owner_true_for_matching_login() -> None: + summary = thread_api._thread_summary( + {"thread_id": "tid", "metadata": {"source": "slack", "github_login": "octocat"}}, + owner_login="octocat", + ) + + assert summary["isOwner"] is True + + +def test_thread_summary_is_owner_false_for_non_owner() -> None: + summary = thread_api._thread_summary( + {"thread_id": "tid", "metadata": {"source": "slack", "github_login": "octocat"}}, + owner_login="teammate", + ) + + assert summary["isOwner"] is False + + +def test_thread_summary_is_owner_true_for_matching_email() -> None: + summary = thread_api._thread_summary( + { + "thread_id": "tid", + "metadata": { + "source": "slack", + "github_login": "octocat", + "triggering_user_email": "octo@example.com", + }, + }, + owner_login="someone-else", + owner_email="OCTO@example.com", + ) + + assert summary["isOwner"] is True + + +def test_thread_summary_is_owner_defaults_true_without_owner_login() -> None: + summary = thread_api._thread_summary( + {"thread_id": "tid", "metadata": {"source": "slack", "github_login": "octocat"}}, + ) + + assert summary["isOwner"] is True + + async def test_resolve_dashboard_thread_marks_resolved(monkeypatch) -> None: updates: list[dict[str, object]] = [] diff --git a/ui/src/components/agents/AgentThreadView.tsx b/ui/src/components/agents/AgentThreadView.tsx index 9397e7ef..9c519d5f 100644 --- a/ui/src/components/agents/AgentThreadView.tsx +++ b/ui/src/components/agents/AgentThreadView.tsx @@ -24,6 +24,7 @@ export function AgentThreadView({ thread }: AgentThreadViewProps) { const sendMessage = useSubmitAgentMessage(thread.id) const stream = useAgentThreadStream() const isMobile = useIsMobile() + const isReadOnly = thread.isOwner === false const { models, defaultSelection } = useModelOptions() const threadSelection = useMemo(() => { @@ -77,10 +78,42 @@ export function AgentThreadView({ thread }: AgentThreadViewProps) { settingUpSandbox={settingUpSandbox} contentWidthClass="max-w-3xl" /> -
-
+ {!isReadOnly && ( +
+
+ + sendMessage.mutateAsync({ + content, + images, + model_id: activeSelection?.modelId ?? null, + effort: activeSelection?.effort ?? null, + }) + } + models={models} + selection={activeSelection} + onSelectionChange={setSelection} + /> +
+
+ )} +
+ ) : isHydrating ? ( +
+

Loading conversation…

+
+ ) : ( +
+

+ This thread has no messages yet. +

+ {!isReadOnly && ( +
@@ -96,35 +129,7 @@ export function AgentThreadView({ thread }: AgentThreadViewProps) { onSelectionChange={setSelection} />
-
-
- ) : isHydrating ? ( -
-

Loading conversation…

-
- ) : ( -
-

- This thread has no messages yet. -

-
- - sendMessage.mutateAsync({ - content, - images, - model_id: activeSelection?.modelId ?? null, - effort: activeSelection?.effort ?? null, - }) - } - models={models} - selection={activeSelection} - onSelectionChange={setSelection} - /> -
+ )}
)} diff --git a/ui/src/components/agents/AgentsSidebar.tsx b/ui/src/components/agents/AgentsSidebar.tsx index ce0ff15e..dad1f38e 100644 --- a/ui/src/components/agents/AgentsSidebar.tsx +++ b/ui/src/components/agents/AgentsSidebar.tsx @@ -315,6 +315,7 @@ function ThreadRow({ const deleteThread = useDeleteAgentThread() const resolveThread = useResolveAgentThread() const [deleteOpen, setDeleteOpen] = useState(false) + const isReadOnly = thread.isOwner === false const badge = thread.diffStats && thread.diffStats.additions > 0 ? `+${thread.diffStats.additions}` @@ -422,29 +423,33 @@ function ThreadRow({ {badge} )} - - + {!isReadOnly && ( + + )} + {!isReadOnly && ( + + )} @@ -457,26 +462,30 @@ function ThreadRow({ Open trace - onToggleResolved()} - disabled={resolveThread.isPending} - className="flex cursor-default items-center gap-2 rounded-sm px-2 py-1.5 text-xs outline-none select-none data-highlighted:bg-[var(--ui-sidebar-hover)] data-disabled:pointer-events-none data-disabled:opacity-50" - > - {isResolved ? ( - - ) : ( - - )} - {isResolved ? "Unresolve thread" : "Resolve thread"} - - - - Delete thread - + {!isReadOnly && ( + onToggleResolved()} + disabled={resolveThread.isPending} + className="flex cursor-default items-center gap-2 rounded-sm px-2 py-1.5 text-xs outline-none select-none data-highlighted:bg-[var(--ui-sidebar-hover)] data-disabled:pointer-events-none data-disabled:opacity-50" + > + {isResolved ? ( + + ) : ( + + )} + {isResolved ? "Unresolve thread" : "Resolve thread"} + + )} + {!isReadOnly && ( + + + Delete thread + + )} diff --git a/ui/src/lib/agents/queries.ts b/ui/src/lib/agents/queries.ts index bd6d177d..f545e146 100644 --- a/ui/src/lib/agents/queries.ts +++ b/ui/src/lib/agents/queries.ts @@ -205,6 +205,7 @@ export function optimisticThread( status: "running", viewed: true, viewedAt: now, + isOwner: true, createdAt: now, updatedAt: now, traceUrl: null, diff --git a/ui/src/lib/agents/types.ts b/ui/src/lib/agents/types.ts index b5357e22..657dfe82 100644 --- a/ui/src/lib/agents/types.ts +++ b/ui/src/lib/agents/types.ts @@ -186,6 +186,7 @@ export interface AgentThread { viewedAt?: number | null resolved?: boolean resolvedAt?: number | null + isOwner?: boolean createdAt: number updatedAt: number traceUrl?: string | null