From 5d033c75485f2824fd07b8be9e2d2cd2d0a7a20c Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Tue, 2 Jun 2026 19:39:44 -0700 Subject: [PATCH] fix: clearer Slack prompt for users who must sign in with GitHub (#1380) * fix: clearer Slack prompt for users who must sign in with GitHub The Slack gate labeled any mapped-but-tokenless user as having an "expired or revoked" authorization, keyed off whether a *mapping* row existed. Most blocked users are team members in the legacy hardcoded mapping who have simply never completed a dashboard GitHub login, so the message was misleading and steered them toward reconnecting Slack (which never creates a token). Base the prompt on whether an oauth_tokens *record* exists: - no record -> ask the user to sign in with GitHub and connect Slack - record but unusable -> ask the user to sign in with GitHub again Add has_access_token_record() to distinguish the two cases. * fix: guard token-record check so a store failure still prompts sign-in The has_access_token_record() lookup ran outside the defensive handling around token resolution. If the store read fails it would raise before posting the sign-in prompt and clearing the Slack assistant status. Wrap it like get_valid_access_token: on failure, default to the sign-in/connect prompt. --- agent/dashboard/profiles.py | 10 +++++++ agent/webapp.py | 51 ++++++++++++++++++++++----------- tests/test_slack_context.py | 56 +++++++++++++++++++++++++++++++++++-- 3 files changed, 97 insertions(+), 20 deletions(-) diff --git a/agent/dashboard/profiles.py b/agent/dashboard/profiles.py index 59309d2f..59a81c2c 100644 --- a/agent/dashboard/profiles.py +++ b/agent/dashboard/profiles.py @@ -270,6 +270,16 @@ async def get_access_token(login: str) -> str | None: return await get_valid_access_token(login) +async def has_access_token_record(login: str) -> bool: + """Whether an OAuth token record exists for ``login``. + + Distinguishes "user has never completed a GitHub login" (no record) from + "the stored authorization is present but no longer usable" (record exists + but won't decrypt / was revoked), so callers can prompt accurately. + """ + return bool(await _get_value(OAUTH_TOKENS_NAMESPACE, login)) + + async def list_profiles() -> list[dict[str, Any]]: result = await _client().store.search_items(PROFILES_NAMESPACE, limit=1000) items = result.get("items") if isinstance(result, dict) else getattr(result, "items", []) diff --git a/agent/webapp.py b/agent/webapp.py index bf2179c6..c721e2f4 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -26,7 +26,7 @@ from .dashboard.agent_overrides import ( ) from .dashboard.enabled_repos import is_review_repo_enabled from .dashboard.oauth import build_account_link_url -from .dashboard.profiles import get_profile, get_valid_access_token +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 ( email_for_login, @@ -875,27 +875,30 @@ async def _post_account_link_prompt( user_email: str | None, reason: str = "unlinked", ) -> None: - """Prompt a Slack user to (re-)link their GitHub account (ephemeral). + """Prompt a Slack user to connect their account via the dashboard (ephemeral). - ``reason`` is ``"unlinked"`` (no mapping yet) or ``"expired"`` (mapped but - the stored GitHub authorization is missing/expired/revoked). Open SWE opens - PRs as the triggering user, so it cannot start until the account is linked. + ``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. """ 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") return - if reason == "expired": + if reason == "revoked": text = ( - "🔐 Your GitHub authorization has expired or was revoked, so I can't act on " - "your behalf. Re-link your account to continue:\n" - f"<{link_url}|Re-link your GitHub account>" + "🔐 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>" ) else: text = ( - "👋 I don't have your GitHub account linked yet, so I can't open PRs on your " - "behalf. I won't start until you link your account:\n" - f"<{link_url}|Link your GitHub account>" + "👋 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>" ) try: await post_slack_ephemeral_message(channel_id, user_id, text, thread_ts=thread_ts) @@ -1024,12 +1027,12 @@ async def process_slack_mention(event_data: dict[str, Any], repo_config: dict[st mapped_login = await login_for_slack_id(user_id) if not mapped_login and user_email: mapped_login = await login_for_email(user_email) - is_user_mapped = bool(mapped_login) # Open SWE opens PRs as the triggering user, so a run only proceeds when we - # have a valid user GitHub token. Unmapped users, and mapped users whose - # token is missing/expired/revoked, are blocked and prompted to (re-)link. - # Bot-token-only deployments are exempt — they run on the installation token. + # have a valid user GitHub token. Users who have never signed in with + # GitHub, and users whose stored authorization is no longer usable, are + # blocked and prompted to set up via the dashboard. Bot-token-only + # deployments are exempt — they run on the installation token. user_token: str | None = None if mapped_login: try: @@ -1044,7 +1047,21 @@ async def process_slack_mention(event_data: dict[str, Any], repo_config: dict[st has_valid_user_token = bool(user_token) if not has_valid_user_token and not is_bot_token_only_mode(): - reason = "expired" if is_user_mapped else "unlinked" + # A stored-but-unusable token means "sign in again"; no record at all + # means the user has never connected GitHub + Slack via the dashboard. + # Guard the store read like token resolution above so a transient + # failure still yields an actionable prompt and clears the status. + has_token_record = False + if mapped_login: + try: + has_token_record = await has_access_token_record(mapped_login) + except Exception: # noqa: BLE001 + logger.debug( + "Failed to check GitHub token record for %s; prompting sign-in", + mapped_login, + exc_info=True, + ) + reason = "revoked" if has_token_record else "unlinked" logger.info( "Blocking Slack run for thread %s: no valid user GitHub token (%s)", thread_id, diff --git a/tests/test_slack_context.py b/tests/test_slack_context.py index 6d54c5f1..1e34b18f 100644 --- a/tests/test_slack_context.py +++ b/tests/test_slack_context.py @@ -764,10 +764,10 @@ def test_process_slack_mention_unmapped_user_blocked_and_prompted( } -def test_process_slack_mention_mapped_user_no_token_blocked_and_prompted( +def test_process_slack_mention_mapped_user_no_token_record_prompts_setup( monkeypatch: pytest.MonkeyPatch, ) -> None: - """A mapped Slack user with no valid token is blocked and prompted to re-link.""" + """A mapped user who never signed in (no token record) is prompted to set up.""" captured: dict[str, object] = {} _setup_slack_mention_fakes(monkeypatch, captured) @@ -780,12 +780,16 @@ def test_process_slack_mention_mapped_user_no_token_blocked_and_prompted( async def fake_get_valid_access_token(login): return None + async def fake_has_token_record(login): + return False + async def fake_post_prompt(channel_id, thread_ts, user_id, user_email, reason="unlinked"): captured["prompt"] = {"reason": reason} monkeypatch.setattr(webapp, "_thread_exists", fake_thread_exists) monkeypatch.setattr(webapp, "login_for_slack_id", fake_login_for_slack_id) monkeypatch.setattr(webapp, "get_valid_access_token", fake_get_valid_access_token) + monkeypatch.setattr(webapp, "has_access_token_record", fake_has_token_record) monkeypatch.setattr(webapp, "_post_account_link_prompt", fake_post_prompt) asyncio.run( @@ -803,7 +807,53 @@ def test_process_slack_mention_mapped_user_no_token_blocked_and_prompted( ) assert "run_create" not in captured - assert captured["prompt"] == {"reason": "expired"} + assert captured["prompt"] == {"reason": "unlinked"} + + +def test_process_slack_mention_mapped_user_unusable_token_prompts_revoked( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A user who signed in before but whose token is now unusable is told to re-auth.""" + captured: dict[str, object] = {} + _setup_slack_mention_fakes(monkeypatch, captured) + + async def fake_thread_exists(thread_id: str) -> bool: + return False + + async def fake_login_for_slack_id(slack_user_id): + return "mason-gh" if slack_user_id == "U123" else None + + async def fake_get_valid_access_token(login): + return None + + async def fake_has_token_record(login): + return True + + async def fake_post_prompt(channel_id, thread_ts, user_id, user_email, reason="unlinked"): + captured["prompt"] = {"reason": reason} + + monkeypatch.setattr(webapp, "_thread_exists", fake_thread_exists) + monkeypatch.setattr(webapp, "login_for_slack_id", fake_login_for_slack_id) + monkeypatch.setattr(webapp, "get_valid_access_token", fake_get_valid_access_token) + monkeypatch.setattr(webapp, "has_access_token_record", fake_has_token_record) + monkeypatch.setattr(webapp, "_post_account_link_prompt", fake_post_prompt) + + asyncio.run( + webapp.process_slack_mention( + { + "channel_id": "C123", + "thread_ts": "1700000000.000100", + "event_ts": "1700000000.000200", + "user_id": "U123", + "text": "<@UBOT> do the thing", + "bot_user_id": "UBOT", + }, + {"owner": "langchain-ai", "name": "open-swe"}, + ) + ) + + assert "run_create" not in captured + assert captured["prompt"] == {"reason": "revoked"} def test_process_slack_mention_mapped_user_with_token_runs_as_user(