diff --git a/agent/dashboard/oauth.py b/agent/dashboard/oauth.py index 62efad89..ae1671cd 100644 --- a/agent/dashboard/oauth.py +++ b/agent/dashboard/oauth.py @@ -205,6 +205,19 @@ def decode_account_link(token: str) -> dict[str, Any] | None: return payload +def build_settings_url() -> str | None: + """Return the dashboard Profile Settings URL, or ``None`` if not configured. + + This is a plain, token-free link: it carries no per-user identity, so it is + safe to share in a public Slack thread. The user signs in with GitHub from + their own session and connects Slack via verified OIDC on the settings page. + """ + frontend_base = os.environ.get("DASHBOARD_BASE_URL", "").rstrip("/") + if not frontend_base: + return None + return f"{frontend_base}{PROFILE_SETTINGS_PATH}" + + def build_account_link_url(*, slack_user_id: str | None, work_email: str | None) -> str | None: """Return the dashboard login URL that links a Slack identity on completion. diff --git a/agent/utils/auth.py b/agent/utils/auth.py index 2a45d1da..caba12eb 100644 --- a/agent/utils/auth.py +++ b/agent/utils/auth.py @@ -17,7 +17,7 @@ from ..encryption import encrypt_token from .github_app import get_github_app_installation_token_with_expiry from .github_token import get_github_token_from_thread from .linear import comment_on_linear_issue -from .slack import post_slack_ephemeral_message, post_slack_thread_reply +from .slack import post_slack_thread_reply logger = logging.getLogger(__name__) @@ -254,39 +254,30 @@ async def leave_failure_comment( slack_thread = configurable.get("slack_thread", {}) channel_id = slack_thread.get("channel_id") if isinstance(slack_thread, dict) else None thread_ts = slack_thread.get("thread_ts") if isinstance(slack_thread, dict) else None - triggering_user_id = ( - slack_thread.get("triggering_user_id") if isinstance(slack_thread, dict) else None - ) if channel_id and thread_ts: - if isinstance(triggering_user_id, str) and triggering_user_id: - logger.info( - "Posting auth failure ephemeral reply to Slack user %s in channel %s thread %s", - triggering_user_id, - channel_id, - thread_ts, - ) - sent = await post_slack_ephemeral_message( - channel_id=channel_id, - user_id=triggering_user_id, - text=message, - thread_ts=thread_ts, - ) - if sent: - return - logger.warning( - "Failed to post ephemeral auth failure reply for Slack user %s; falling back to thread reply", - triggering_user_id, - ) - else: - logger.warning( - "Missing Slack triggering_user_id for auth failure reply; falling back to thread reply", - ) + # The auth-failure ``message`` can carry a per-user GitHub auth URL, + # which must not be posted in a shared thread (anyone could complete + # it and bind the wrong account). Post a generic, token-free notice and + # let the user finish sign-in from their own authenticated dashboard. + from ..dashboard.oauth import build_settings_url + + settings_url = build_settings_url() + link = ( + f"<{settings_url}|your Open SWE settings>" + if settings_url + else "your Open SWE settings" + ) logger.info( - "Posting auth failure reply to Slack channel %s thread %s", + "Posting generic auth-failure notice to Slack channel %s thread %s", channel_id, thread_ts, ) - await post_slack_thread_reply(channel_id, thread_ts, message) + await post_slack_thread_reply( + channel_id, + thread_ts, + "⚠️ I couldn't resolve your GitHub account for this run. Sign in with GitHub and " + f"connect your Slack account in {link}, then tag me again.", + ) return if source == "github": logger.warning( diff --git a/agent/webapp.py b/agent/webapp.py index c721e2f4..f38dea4d 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -25,7 +25,7 @@ from .dashboard.agent_overrides import ( resolve_login_from_email_async, ) from .dashboard.enabled_repos import is_review_repo_enabled -from .dashboard.oauth import build_account_link_url +from .dashboard.oauth import build_settings_url from .dashboard.profiles import get_profile, get_valid_access_token, has_access_token_record from .dashboard.team_settings import get_team_settings from .dashboard.user_mappings import ( @@ -89,7 +89,6 @@ from .utils.slack import ( get_slack_user_info, get_slack_user_names, parse_github_pr_url, - post_slack_ephemeral_message, post_slack_thread_reply, post_slack_trace_reply, resolve_slack_links_in_context, @@ -875,33 +874,37 @@ async def _post_account_link_prompt( user_email: str | None, reason: str = "unlinked", ) -> None: - """Prompt a Slack user to connect their account via the dashboard (ephemeral). + """Prompt a Slack user to connect their account via the dashboard. ``reason`` is ``"unlinked"`` (never signed in with GitHub) or ``"revoked"`` (signed in before, but the stored GitHub authorization is no longer usable). Open SWE opens PRs as the triggering user, so it cannot start until the user has signed in with GitHub and connected their Slack account in the dashboard. - The link runs the GitHub sign-in and lands them on Profile Settings, where - they can connect Slack. + + Posts a plain, token-free dashboard link as a visible threaded reply. The + link carries no per-user identity, so it's safe to show in a shared channel: + the user signs in with GitHub from their own session and connects Slack via + verified OIDC on the settings page. """ - link_url = build_account_link_url(slack_user_id=user_id, work_email=user_email) - if not link_url: - logger.debug("Account-link URL unavailable (DASHBOARD_API_BASE_URL unset); skipping prompt") + settings_url = build_settings_url() + if not settings_url: + logger.debug( + "Dashboard settings URL unavailable (DASHBOARD_BASE_URL unset); skipping prompt" + ) return if reason == "revoked": text = ( - "🔐 Your GitHub sign-in is no longer valid, so I can't act on your behalf. " - "Sign in with GitHub again to reconnect:\n" - f"<{link_url}|Sign in with GitHub>" + "🔐 Your GitHub sign-in is no longer valid, so I can't resolve your GitHub " + f"account. Re-connect it in <{settings_url}|your Open SWE settings>, then tag me again." ) else: text = ( - "👋 To act on your behalf I need you to sign in with GitHub and connect your " - "Slack account. Set that up in your dashboard:\n" - f"<{link_url}|Sign in with GitHub & connect Slack>" + "👋 I couldn't resolve your GitHub account from Slack. Sign in with GitHub and " + f"connect your Slack account in <{settings_url}|your Open SWE settings>, then tag me " + "again." ) try: - await post_slack_ephemeral_message(channel_id, user_id, text, thread_ts=thread_ts) + await post_slack_thread_reply(channel_id, thread_ts, text) except Exception: # noqa: BLE001 logger.debug("Failed to post account-link prompt to Slack", exc_info=True) diff --git a/tests/test_account_link.py b/tests/test_account_link.py index 8133854f..dda3dda6 100644 --- a/tests/test_account_link.py +++ b/tests/test_account_link.py @@ -62,3 +62,68 @@ def test_build_account_link_url_redirects_to_profile_settings( def test_build_account_link_url_none_without_base(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("DASHBOARD_API_BASE_URL", raising=False) assert oauth.build_account_link_url(slack_user_id="U1", work_email="d@x.com") is None + + +def test_account_link_prompt_posts_generic_token_free_link( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The prompt posts a plain settings link in the thread — no per-user token.""" + import asyncio + + from agent import webapp + + monkeypatch.setenv("DASHBOARD_BASE_URL", "https://app.example.com") + calls: dict[str, object] = {} + + async def fake_reply(channel_id, thread_ts, text): + calls["reply"] = {"channel_id": channel_id, "thread_ts": thread_ts, "text": text} + return True + + monkeypatch.setattr(webapp, "post_slack_thread_reply", fake_reply) + + asyncio.run(webapp._post_account_link_prompt("C1", "1.1", "U1", "d@x.com", reason="unlinked")) + assert calls["reply"]["channel_id"] == "C1" + assert calls["reply"]["thread_ts"] == "1.1" + assert "https://app.example.com/my-settings" in calls["reply"]["text"] + # No signed account-link token may appear in the public thread. + assert "link=" not in calls["reply"]["text"] + + +def test_account_link_prompt_revoked_wording(monkeypatch: pytest.MonkeyPatch) -> None: + import asyncio + + from agent import webapp + + monkeypatch.setenv("DASHBOARD_BASE_URL", "https://app.example.com") + calls: dict[str, object] = {} + + async def fake_reply(channel_id, thread_ts, text): + calls["text"] = text + return True + + monkeypatch.setattr(webapp, "post_slack_thread_reply", fake_reply) + + asyncio.run(webapp._post_account_link_prompt("C1", "1.1", "U1", "d@x.com", reason="revoked")) + assert "no longer valid" in calls["text"] + assert "link=" not in calls["text"] + + +def test_account_link_prompt_skips_when_dashboard_url_unset( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import asyncio + + from agent import webapp + + monkeypatch.delenv("DASHBOARD_BASE_URL", raising=False) + posted = False + + async def fake_reply(channel_id, thread_ts, text): + nonlocal posted + posted = True + return True + + monkeypatch.setattr(webapp, "post_slack_thread_reply", fake_reply) + + asyncio.run(webapp._post_account_link_prompt("C1", "1.1", "U1", "d@x.com", reason="unlinked")) + assert posted is False diff --git a/tests/test_auth_sources.py b/tests/test_auth_sources.py index dd42fea6..23b05881 100644 --- a/tests/test_auth_sources.py +++ b/tests/test_auth_sources.py @@ -7,66 +7,19 @@ import pytest from agent.utils import auth -def test_leave_failure_comment_posts_to_slack_thread( - monkeypatch: pytest.MonkeyPatch, -) -> None: - called: dict[str, str] = {} - - async def fake_post_slack_ephemeral_message( - channel_id: str, user_id: str, text: str, thread_ts: str | None = None - ) -> bool: - called["channel_id"] = channel_id - called["user_id"] = user_id - called["thread_ts"] = thread_ts - called["message"] = text - return True - - async def fake_post_slack_thread_reply(channel_id: str, thread_ts: str, message: str) -> bool: - raise AssertionError("post_slack_thread_reply should not be called when ephemeral succeeds") - - monkeypatch.setattr(auth, "post_slack_ephemeral_message", fake_post_slack_ephemeral_message) - monkeypatch.setattr(auth, "post_slack_thread_reply", fake_post_slack_thread_reply) - monkeypatch.setattr( - auth, - "get_config", - lambda: { - "configurable": { - "slack_thread": { - "channel_id": "C123", - "thread_ts": "1.2", - "triggering_user_id": "U123", - } - } - }, - ) - - asyncio.run(auth.leave_failure_comment("slack", "auth failed")) - - assert called == { - "channel_id": "C123", - "user_id": "U123", - "thread_ts": "1.2", - "message": "auth failed", - } - - -def test_leave_failure_comment_falls_back_to_slack_thread_when_ephemeral_fails( +def test_leave_failure_comment_posts_generic_token_free_slack_notice( monkeypatch: pytest.MonkeyPatch, ) -> None: + """Slack auth failures post a generic notice, never the (possibly sensitive) message.""" + monkeypatch.setenv("DASHBOARD_BASE_URL", "https://app.example.com") thread_called: dict[str, str] = {} - async def fake_post_slack_ephemeral_message( - channel_id: str, user_id: str, text: str, thread_ts: str | None = None - ) -> bool: - return False - async def fake_post_slack_thread_reply(channel_id: str, thread_ts: str, message: str) -> bool: thread_called["channel_id"] = channel_id thread_called["thread_ts"] = thread_ts thread_called["message"] = message return True - monkeypatch.setattr(auth, "post_slack_ephemeral_message", fake_post_slack_ephemeral_message) monkeypatch.setattr(auth, "post_slack_thread_reply", fake_post_slack_thread_reply) monkeypatch.setattr( auth, @@ -82,9 +35,13 @@ def test_leave_failure_comment_falls_back_to_slack_thread_when_ephemeral_fails( }, ) - asyncio.run(auth.leave_failure_comment("slack", "auth failed")) + # Pass a message that embeds a per-user auth URL; it must NOT be echoed publicly. + asyncio.run(auth.leave_failure_comment("slack", "Click https://auth.example/secret-token")) - assert thread_called == {"channel_id": "C123", "thread_ts": "1.2", "message": "auth failed"} + assert thread_called["channel_id"] == "C123" + assert thread_called["thread_ts"] == "1.2" + assert "secret-token" not in thread_called["message"] + assert "https://app.example.com/my-settings" in thread_called["message"] def _slack_config(github_login: str | None = "mason-gh") -> dict: diff --git a/ui/src/components/agents/AgentsHome.tsx b/ui/src/components/agents/AgentsHome.tsx index f0d60651..ce2f69c0 100644 --- a/ui/src/components/agents/AgentsHome.tsx +++ b/ui/src/components/agents/AgentsHome.tsx @@ -1,25 +1,28 @@ -import { Link } from "@tanstack/react-router"; -import { useEffect, useState } from "react"; +import { Link } from "@tanstack/react-router" +import { useEffect, useState } from "react" -import { AgentPromptBar } from "@/components/agents/AgentPromptBar"; -import { AgentRunCard } from "@/components/agents/AgentRunCard"; -import { Logo } from "@/components/agents/ported/Logo"; -import { useAgentThreads, useCreateAgentThread } from "@/lib/agents/queries"; -import { useModelOptions, type ModelSelection } from "@/lib/agents/useModelOptions"; +import type { ModelSelection } from "@/lib/agents/useModelOptions" +import { AgentPromptBar } from "@/components/agents/AgentPromptBar" +import { AgentRunCard } from "@/components/agents/AgentRunCard" +import { SlackConnectDialog } from "@/components/agents/SlackConnectDialog" +import { Logo } from "@/components/agents/ported/Logo" +import { useAgentThreads, useCreateAgentThread } from "@/lib/agents/queries" +import { useModelOptions } from "@/lib/agents/useModelOptions" export function AgentsHome() { - const threadsQuery = useAgentThreads(); - const createThread = useCreateAgentThread(); - const recentRuns = (threadsQuery.data ?? []).slice(0, 5); - const { models, defaultSelection } = useModelOptions(); - const [selection, setSelection] = useState(null); + const threadsQuery = useAgentThreads() + const createThread = useCreateAgentThread() + const recentRuns = (threadsQuery.data ?? []).slice(0, 5) + const { models, defaultSelection } = useModelOptions() + const [selection, setSelection] = useState(null) useEffect(() => { - if (selection === null && defaultSelection) setSelection(defaultSelection); - }, [defaultSelection, selection]); + if (selection === null && defaultSelection) setSelection(defaultSelection) + }, [defaultSelection, selection]) return (
+
@@ -40,16 +43,20 @@ export function AgentsHome() {
{threadsQuery.isLoading ? ( -

Loading agents…

+

+ Loading agents… +

) : recentRuns.length === 0 ? ( ) : ( - recentRuns.map((thread) => ) + recentRuns.map((thread) => ( + + )) )}
- ); + ) } export function AgentsHomeEmptyState() { @@ -63,5 +70,5 @@ export function AgentsHomeEmptyState() { Start your first agent
- ); + ) } diff --git a/ui/src/components/agents/SlackConnectDialog.tsx b/ui/src/components/agents/SlackConnectDialog.tsx new file mode 100644 index 00000000..6ddcbb86 --- /dev/null +++ b/ui/src/components/agents/SlackConnectDialog.tsx @@ -0,0 +1,71 @@ +import { Dialog } from "@base-ui/react/dialog" +import { useQuery } from "@tanstack/react-query" +import { useState } from "react" +import { IoLogoSlack } from "react-icons/io5" + +import { Button } from "@/components/ui/button" +import { api, slackConnectUrl } from "@/lib/api" +import { useSession } from "@/lib/session" + +/** + * Modal shown on first login (and until connected) prompting the user to link + * Slack so Open SWE can resolve their GitHub account when tagged in Slack. + * ``open`` is derived from the mapping query, so it appears once the data + * resolves to "not connected" and closes itself once Slack is linked; dismissing + * it hides it for the session. + */ +export function SlackConnectDialog() { + const session = useSession() + const mapping = useQuery({ queryKey: ["myMapping"], queryFn: api.myMapping }) + const [dismissed, setDismissed] = useState(false) + + const slackEnabled = session.data?.slack_oauth_enabled ?? false + const connected = !!mapping.data?.slack_user_id + const shouldShow = + slackEnabled && !connected && !mapping.isLoading && !mapping.isError + const open = shouldShow && !dismissed + + return ( + { + if (!next) setDismissed(true) + }} + > + + + +
+
+ + + Connect your Slack account + +
+ + Connect Slack so that when you tag Open SWE, it can resolve your + GitHub account. We use the email Slack verifies, which also lets + Linear mentions resolve to you. + +
+ + +
+
+ + + + ) +} diff --git a/ui/src/routes/my-settings.tsx b/ui/src/routes/my-settings.tsx index 182fe0af..cc0708b1 100644 --- a/ui/src/routes/my-settings.tsx +++ b/ui/src/routes/my-settings.tsx @@ -1,65 +1,76 @@ -import { Navigate, createFileRoute, useNavigate } from "@tanstack/react-router"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; -import { useState } from "react"; -import { IoLogoSlack } from "react-icons/io5"; +import { Navigate, createFileRoute, useNavigate } from "@tanstack/react-router" +import { useQuery, useQueryClient } from "@tanstack/react-query" +import { useState } from "react" +import { IoLogoSlack } from "react-icons/io5" -import type { SessionUser } from "@/lib/api"; -import { AppShell, SettingsRow, SettingsSection } from "@/components/AppShell"; -import { Button } from "@/components/ui/button"; +import type { SessionUser } from "@/lib/api" +import { AppShell, SettingsRow, SettingsSection } from "@/components/AppShell" +import { Button } from "@/components/ui/button" import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue, -} from "@/components/ui/select"; -import { Skeleton } from "@/components/ui/skeleton"; -import { api, slackConnectUrl } from "@/lib/api"; -import { buildProfileUpdate, useOptions, useProfile, useSaveProfile } from "@/lib/profile"; -import { useSession } from "@/lib/session"; -import { cn } from "@/lib/utils"; +} from "@/components/ui/select" +import { Skeleton } from "@/components/ui/skeleton" +import { api, slackConnectUrl } from "@/lib/api" +import { + buildProfileUpdate, + useOptions, + useProfile, + useSaveProfile, +} from "@/lib/profile" +import { useSession } from "@/lib/session" +import { cn } from "@/lib/utils" -export const Route = createFileRoute("/my-settings")({ component: MySettingsPage }); +export const Route = createFileRoute("/my-settings")({ + component: MySettingsPage, +}) -type DraftReviewChoice = "team_default" | "always_on" | "always_off"; +type DraftReviewChoice = "team_default" | "always_on" | "always_off" function toChoice(value: boolean | null | undefined): DraftReviewChoice { - if (value === true) return "always_on"; - if (value === false) return "always_off"; - return "team_default"; + if (value === true) return "always_on" + if (value === false) return "always_off" + return "team_default" } function fromChoice(choice: DraftReviewChoice): boolean | null { - if (choice === "always_on") return true; - if (choice === "always_off") return false; - return null; + if (choice === "always_on") return true + if (choice === "always_off") return false + return null } function UserMappingSection({ session }: { session: SessionUser }) { - const qc = useQueryClient(); - const mapping = useQuery({ queryKey: ["myMapping"], queryFn: api.myMapping }); - const [connecting, setConnecting] = useState(false); + const qc = useQueryClient() + const mapping = useQuery({ queryKey: ["myMapping"], queryFn: api.myMapping }) + const [connecting, setConnecting] = useState(false) - const slackUserId = mapping.data?.slack_user_id ?? null; - const workEmail = mapping.data?.work_email ?? null; - const connected = !!slackUserId; + const slackUserId = mapping.data?.slack_user_id ?? null + const workEmail = mapping.data?.work_email ?? null + const connected = !!slackUserId const connect = () => { - setConnecting(true); + setConnecting(true) // Refresh the cached mapping when the user returns from the OAuth redirect. - void qc.invalidateQueries({ queryKey: ["myMapping"] }); - window.location.assign(slackConnectUrl()); - }; + void qc.invalidateQueries({ queryKey: ["myMapping"] }) + window.location.assign(slackConnectUrl()) + } return (
{session.login}} + control={ + + {session.login} + + } /> {connected ? "Connected" : "Not connected"} @@ -86,69 +99,75 @@ function UserMappingSection({ session }: { session: SessionUser }) { disabled={connecting || mapping.isLoading} > - {connecting ? "Redirecting…" : connected ? "Reconnect" : "Connect Slack"} + {connecting + ? "Redirecting…" + : connected + ? "Reconnect" + : "Connect Slack"} ) : ( - Sign in with Slack unavailable + + Sign in with Slack unavailable + )}
} /> - ); + ) } function MySettingsPage() { - const session = useSession(); - const qc = useQueryClient(); - const navigate = useNavigate(); - const profile = useProfile(); - const options = useOptions(); - const save = useSaveProfile(); + const session = useSession() + const qc = useQueryClient() + const navigate = useNavigate() + const profile = useProfile() + const options = useOptions() + const save = useSaveProfile() const teamSettings = useQuery({ queryKey: ["teamSettings"], queryFn: api.getTeamSettings, enabled: !!session.data, - }); - const [error, setError] = useState(null); + }) + const [error, setError] = useState(null) if (session.isLoading) { return (
- ); + ) } - if (!session.data) return ; + if (!session.data) return const handleLogout = async () => { - await api.logout(); - qc.setQueryData(["session"], null); - void navigate({ to: "/login" }); - }; + await api.logout() + qc.setQueryData(["session"], null) + void navigate({ to: "/login" }) + } - const firstModel = options.data?.models[0]; - const fallbackModel = firstModel?.id ?? ""; - const fallbackEffort = firstModel?.default_effort ?? ""; + const firstModel = options.data?.models[0] + const fallbackModel = firstModel?.id ?? "" + const fallbackEffort = firstModel?.default_effort ?? "" - const draftChoice = toChoice(profile.data?.review_draft_prs); - const teamDefaultOn = teamSettings.data?.review_draft_prs ?? false; - const teamDefaultLabel = `Use team default (currently: ${teamDefaultOn ? "On" : "Off"})`; + const draftChoice = toChoice(profile.data?.review_draft_prs) + const teamDefaultOn = teamSettings.data?.review_draft_prs ?? false + const teamDefaultLabel = `Use team default (currently: ${teamDefaultOn ? "On" : "Off"})` const handleDraftChoiceChange = (next: DraftReviewChoice) => { - setError(null); + setError(null) save .mutateAsync( buildProfileUpdate( profile.data, { review_draft_prs: fromChoice(next) }, fallbackModel, - fallbackEffort, - ), + fallbackEffort + ) ) - .catch((e: Error) => setError(e.message)); - }; + .catch((e: Error) => setError(e.message)) + } return ( @@ -156,7 +175,9 @@ function MySettingsPage() { {session.data.email ?? "—"} + + {session.data.email ?? "—"} + } /> @@ -170,7 +191,9 @@ function MySettingsPage() { control={ } @@ -191,7 +218,11 @@ function MySettingsPage() { label="Sign out" description="End your dashboard session." control={ - } @@ -200,5 +231,5 @@ function MySettingsPage() { {error &&

{error}

} - ); + ) }