mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 09:13:14 +00:00
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.
This commit is contained in:
parent
04c346176d
commit
5d033c7548
3 changed files with 97 additions and 20 deletions
|
|
@ -270,6 +270,16 @@ async def get_access_token(login: str) -> str | None:
|
||||||
return await get_valid_access_token(login)
|
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]]:
|
async def list_profiles() -> list[dict[str, Any]]:
|
||||||
result = await _client().store.search_items(PROFILES_NAMESPACE, limit=1000)
|
result = await _client().store.search_items(PROFILES_NAMESPACE, limit=1000)
|
||||||
items = result.get("items") if isinstance(result, dict) else getattr(result, "items", [])
|
items = result.get("items") if isinstance(result, dict) else getattr(result, "items", [])
|
||||||
|
|
|
||||||
|
|
@ -26,7 +26,7 @@ from .dashboard.agent_overrides import (
|
||||||
)
|
)
|
||||||
from .dashboard.enabled_repos import is_review_repo_enabled
|
from .dashboard.enabled_repos import is_review_repo_enabled
|
||||||
from .dashboard.oauth import build_account_link_url
|
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.team_settings import get_team_settings
|
||||||
from .dashboard.user_mappings import (
|
from .dashboard.user_mappings import (
|
||||||
email_for_login,
|
email_for_login,
|
||||||
|
|
@ -875,27 +875,30 @@ async def _post_account_link_prompt(
|
||||||
user_email: str | None,
|
user_email: str | None,
|
||||||
reason: str = "unlinked",
|
reason: str = "unlinked",
|
||||||
) -> None:
|
) -> 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
|
``reason`` is ``"unlinked"`` (never signed in with GitHub) or ``"revoked"``
|
||||||
the stored GitHub authorization is missing/expired/revoked). Open SWE opens
|
(signed in before, but the stored GitHub authorization is no longer usable).
|
||||||
PRs as the triggering user, so it cannot start until the account is linked.
|
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)
|
link_url = build_account_link_url(slack_user_id=user_id, work_email=user_email)
|
||||||
if not link_url:
|
if not link_url:
|
||||||
logger.debug("Account-link URL unavailable (DASHBOARD_API_BASE_URL unset); skipping prompt")
|
logger.debug("Account-link URL unavailable (DASHBOARD_API_BASE_URL unset); skipping prompt")
|
||||||
return
|
return
|
||||||
if reason == "expired":
|
if reason == "revoked":
|
||||||
text = (
|
text = (
|
||||||
"🔐 Your GitHub authorization has expired or was revoked, so I can't act on "
|
"🔐 Your GitHub sign-in is no longer valid, so I can't act on your behalf. "
|
||||||
"your behalf. Re-link your account to continue:\n"
|
"Sign in with GitHub again to reconnect:\n"
|
||||||
f"<{link_url}|Re-link your GitHub account>"
|
f"<{link_url}|Sign in with GitHub>"
|
||||||
)
|
)
|
||||||
else:
|
else:
|
||||||
text = (
|
text = (
|
||||||
"👋 I don't have your GitHub account linked yet, so I can't open PRs on your "
|
"👋 To act on your behalf I need you to sign in with GitHub and connect your "
|
||||||
"behalf. I won't start until you link your account:\n"
|
"Slack account. Set that up in your dashboard:\n"
|
||||||
f"<{link_url}|Link your GitHub account>"
|
f"<{link_url}|Sign in with GitHub & connect Slack>"
|
||||||
)
|
)
|
||||||
try:
|
try:
|
||||||
await post_slack_ephemeral_message(channel_id, user_id, text, thread_ts=thread_ts)
|
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)
|
mapped_login = await login_for_slack_id(user_id)
|
||||||
if not mapped_login and user_email:
|
if not mapped_login and user_email:
|
||||||
mapped_login = await login_for_email(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
|
# 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
|
# have a valid user GitHub token. Users who have never signed in with
|
||||||
# token is missing/expired/revoked, are blocked and prompted to (re-)link.
|
# GitHub, and users whose stored authorization is no longer usable, are
|
||||||
# Bot-token-only deployments are exempt — they run on the installation token.
|
# 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
|
user_token: str | None = None
|
||||||
if mapped_login:
|
if mapped_login:
|
||||||
try:
|
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)
|
has_valid_user_token = bool(user_token)
|
||||||
|
|
||||||
if not has_valid_user_token and not is_bot_token_only_mode():
|
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(
|
logger.info(
|
||||||
"Blocking Slack run for thread %s: no valid user GitHub token (%s)",
|
"Blocking Slack run for thread %s: no valid user GitHub token (%s)",
|
||||||
thread_id,
|
thread_id,
|
||||||
|
|
|
||||||
|
|
@ -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,
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
) -> None:
|
) -> 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] = {}
|
captured: dict[str, object] = {}
|
||||||
_setup_slack_mention_fakes(monkeypatch, captured)
|
_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):
|
async def fake_get_valid_access_token(login):
|
||||||
return None
|
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"):
|
async def fake_post_prompt(channel_id, thread_ts, user_id, user_email, reason="unlinked"):
|
||||||
captured["prompt"] = {"reason": reason}
|
captured["prompt"] = {"reason": reason}
|
||||||
|
|
||||||
monkeypatch.setattr(webapp, "_thread_exists", fake_thread_exists)
|
monkeypatch.setattr(webapp, "_thread_exists", fake_thread_exists)
|
||||||
monkeypatch.setattr(webapp, "login_for_slack_id", fake_login_for_slack_id)
|
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, "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)
|
monkeypatch.setattr(webapp, "_post_account_link_prompt", fake_post_prompt)
|
||||||
|
|
||||||
asyncio.run(
|
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 "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(
|
def test_process_slack_mention_mapped_user_with_token_runs_as_user(
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue