diff --git a/src/openjarvis/connectors/gmail.py b/src/openjarvis/connectors/gmail.py index 15858738..72c3315c 100644 --- a/src/openjarvis/connectors/gmail.py +++ b/src/openjarvis/connectors/gmail.py @@ -497,6 +497,11 @@ class GmailConnector(BaseConnector): timestamp=timestamp, thread_id=thread_id, channel=channel, + # Deep-link straight to the message. ``msg_id`` is Gmail's + # internal hex id, which the ``#all/`` permalink + # resolves directly — so citations have a working URL + # without relying on _hit_url reconstruction at query time. + url=f"https://mail.google.com/mail/u/0/#all/{msg_id}", metadata={ "message_id": msg_id, "rfc_message_id": rfc_message_id, diff --git a/src/openjarvis/connectors/granola.py b/src/openjarvis/connectors/granola.py index 45624933..fd92b4e4 100644 --- a/src/openjarvis/connectors/granola.py +++ b/src/openjarvis/connectors/granola.py @@ -76,6 +76,41 @@ def _granola_api_list_notes( return resp.json() +class GranolaKeyError(ValueError): + """Raised when a Granola API key is missing or rejected by the API. + + Surfaced through the ``/connect`` endpoint as an HTTP 400 so the user + sees why the key was refused instead of a silent failed sync later. + """ + + +def _granola_api_validate_key(api_key: str) -> None: + """Verify an API key with a minimal ``GET /v1/notes?limit=1`` probe. + + Raises :class:`GranolaKeyError` when the key is empty or the API + responds 401/403, so an invalid key never overwrites a working + credential on disk. Other HTTP errors propagate via ``raise_for_status``. + """ + if not api_key: + raise GranolaKeyError("Granola API key is empty.") + try: + resp = httpx.get( + f"{_GRANOLA_API_BASE}/v1/notes", + headers={"Authorization": f"Bearer {api_key}"}, + params={"limit": 1}, + timeout=30.0, + ) + except httpx.HTTPError as exc: + raise GranolaKeyError( + f"Could not reach Granola to verify the key: {exc}" + ) from exc + if resp.status_code in (401, 403): + raise GranolaKeyError( + "Invalid API key. Check your key in Granola Settings → API." + ) + resp.raise_for_status() + + def _granola_api_get_note(api_key: str, note_id: str) -> Dict[str, Any]: """Fetch a single Granola note by ID (includes transcript). @@ -242,10 +277,15 @@ class GranolaConnector(BaseConnector): ) def handle_callback(self, code: str) -> None: - """Persist the API key to the credentials file. + """Validate and persist the API key. - The *code* parameter holds the raw API key string provided by the user. + The *code* parameter holds the raw API key string provided by the + user. The key is verified with a live ``GET /v1/notes?limit=1`` + probe *before* it is written, so an invalid key can never overwrite + a working credential on disk (raises :class:`GranolaKeyError` on a + 401/403). """ + _granola_api_validate_key(code) save_tokens(self._credentials_path, {"token": code}) def sync( diff --git a/src/openjarvis/connectors/slack_connector.py b/src/openjarvis/connectors/slack_connector.py index 8f901594..ca42abea 100644 --- a/src/openjarvis/connectors/slack_connector.py +++ b/src/openjarvis/connectors/slack_connector.py @@ -205,17 +205,13 @@ def _validate_user_token(token: str) -> None: raise SlackTokenError("Slack token is empty.") if token.startswith(_BOT_TOKEN_PREFIX): raise SlackTokenError( - "Slack bot tokens (xoxb-) can't see user-to-user DMs. " - "Use a User OAuth Token (xoxp-) from " - "api.slack.com/apps → OAuth & Permissions, " - "with User Token Scopes channels:history, channels:read, " - "groups:history, groups:read, im:history, im:read, " - "mpim:history, mpim:read, users:read." + "Bot tokens (xoxb-) can't read DMs. " + "Use a User OAuth Token (xoxp-) instead." ) if not token.startswith(_USER_TOKEN_PREFIX): raise SlackTokenError( - "Slack token must be a User OAuth Token (starts with 'xoxp-'). " - "Got a token with an unexpected prefix." + "Invalid token format. Expected a Slack User OAuth Token " + "starting with xoxp-" ) @@ -337,14 +333,35 @@ class SlackConnector(BaseConnector): return f"{_SLACK_AUTH_ENDPOINT}?{urlencode(params)}" def handle_callback(self, code: str) -> None: - """Persist the supplied User OAuth Token after validating its shape. + """Validate and persist a supplied User OAuth Token. The connector ``/connect`` endpoint funnels manually-pasted tokens through this method (the parameter is named ``code`` for OAuth-flow - compatibility). Bot tokens (``xoxb-``) are rejected here so the - invalid credential never lands on disk. + compatibility). The token is checked two ways before it is allowed + to touch disk, so an invalid credential never overwrites a working + one: + + 1. **Shape** — must start with ``xoxp-`` (``xoxb-`` bot tokens and + any other prefix are rejected via :func:`_validate_user_token`). + 2. **Liveness** — a live ``auth.test`` call must return ``ok`` so an + expired or revoked token is caught at connect time. """ _validate_user_token(code) + + # Verify the token actually works against Slack before persisting. + try: + auth_resp = _slack_api_auth_test(code) + except Exception as exc: # noqa: BLE001 — surface as a token error + raise SlackTokenError( + f"Could not verify the Slack token (auth.test failed: {exc})." + ) from exc + if not auth_resp.get("ok", False): + err = str(auth_resp.get("error", "auth_failed")) + raise SlackTokenError( + f"Slack rejected the token (auth.test: {err}). " + "Check that it is a current User OAuth Token." + ) + save_tokens(self._credentials_path, {"token": code}) self._last_error = None diff --git a/src/openjarvis/server/connectors_router.py b/src/openjarvis/server/connectors_router.py index a61bf58b..7f02b067 100644 --- a/src/openjarvis/server/connectors_router.py +++ b/src/openjarvis/server/connectors_router.py @@ -320,9 +320,20 @@ def create_connectors_router(): if req.code: instance.handle_callback(req.code) elif req.token: - # Some OAuth connectors accept a pre-existing token. + # A credential pasted into the ``token`` field. Connectors + # that accept a pre-existing access token expose ``_token`` + # and set it directly (their real OAuth code-exchange runs + # via /oauth/start → /oauth/callback). Connectors that + # persist a manually-supplied credential — the Slack user + # token and the Granola API key — validate inside + # handle_callback, so route through it to guarantee the + # credential is verified before anything touches disk. A + # failed validation raises and is turned into HTTP 400 + # below, leaving any existing credential intact. if hasattr(instance, "_token"): instance._token = req.token + else: + instance.handle_callback(req.token) else: # Generic: try to store token or credentials if the instance diff --git a/tests/connectors/test_gmail.py b/tests/connectors/test_gmail.py index d9bc1830..b89728c9 100644 --- a/tests/connectors/test_gmail.py +++ b/tests/connectors/test_gmail.py @@ -150,6 +150,9 @@ def test_sync_yields_documents( assert doc1.content == "Hello world" assert doc1.thread_id == "thread1" assert "alice@example.com" in doc1.participants + # Deep-link permalink to the message must be populated at ingest time + # (GH #408) — not left empty for _hit_url to reconstruct later. + assert doc1.url == "https://mail.google.com/mail/u/0/#all/msg1" # --- Message 2 --- doc2 = next(d for d in docs if d.doc_id == "gmail:msg2") @@ -157,6 +160,7 @@ def test_sync_yields_documents( assert doc2.author == "bob@example.com" assert doc2.content == "Budget reply" assert doc2.thread_id == "thread2" + assert doc2.url == "https://mail.google.com/mail/u/0/#all/msg2" # Verify the API was called correctly mock_list.assert_called_once() diff --git a/tests/connectors/test_granola.py b/tests/connectors/test_granola.py index 1c5fd896..be647cfc 100644 --- a/tests/connectors/test_granola.py +++ b/tests/connectors/test_granola.py @@ -375,3 +375,88 @@ def test_end_to_end_ingest_and_search( # And the client-facing sources list does end up with the stored URL. client_sources = build_sources_for_client([target]) assert client_sources[0]["url"] == _NOTE_1_WEB_URL + + +# --------------------------------------------------------------------------- +# Test — API-key validation happens BEFORE the key is written to disk, so an +# invalid key is rejected at connect time and can never overwrite a working +# credential (the data-loss bug this guards against, GH #409). +# --------------------------------------------------------------------------- + + +class _FakeResponse: + """Minimal httpx.Response stand-in for the validation probe.""" + + def __init__(self, status_code: int) -> None: + self.status_code = status_code + + def raise_for_status(self) -> None: + if self.status_code >= 400: + import httpx # noqa: PLC0415 + + raise httpx.HTTPStatusError( + "error", request=None, response=None # type: ignore[arg-type] + ) + + +def test_validate_key_empty_raises() -> None: + """An empty key is rejected without any network call.""" + from openjarvis.connectors.granola import ( # noqa: PLC0415 + GranolaKeyError, + _granola_api_validate_key, + ) + + with pytest.raises(GranolaKeyError): + _granola_api_validate_key("") + + +@pytest.mark.parametrize("status", [401, 403]) +def test_validate_key_rejects_unauthorized(status: int) -> None: + """A 401/403 from GET /v1/notes raises GranolaKeyError with guidance.""" + from openjarvis.connectors.granola import ( # noqa: PLC0415 + GranolaKeyError, + _granola_api_validate_key, + ) + + with patch( + "openjarvis.connectors.granola.httpx.get", + return_value=_FakeResponse(status), + ) as mock_get: + with pytest.raises(GranolaKeyError) as excinfo: + _granola_api_validate_key("grl_bad_key") + + assert str(excinfo.value) == ( + "Invalid API key. Check your key in Granola Settings → API." + ) + # The probe must hit GET /v1/notes with limit=1 (cheap validation call). + _, kwargs = mock_get.call_args + assert kwargs["params"] == {"limit": 1} + + +@patch("openjarvis.connectors.granola._granola_api_validate_key") +def test_handle_callback_persists_after_validation(mock_validate, connector) -> None: + """A valid key is written only after the validation probe succeeds.""" + connector.handle_callback("grl_good_key") + + mock_validate.assert_called_once_with("grl_good_key") + stored = json.loads(Path(connector._credentials_path).read_text()) + assert stored["token"] == "grl_good_key" + + +def test_handle_callback_invalid_key_does_not_overwrite_existing(connector) -> None: + """A bad key must not clobber an existing, working credential on disk.""" + from openjarvis.connectors.granola import GranolaKeyError # noqa: PLC0415 + + creds_path = Path(connector._credentials_path) + creds_path.write_text(json.dumps({"token": "grl_real_existing_key"})) + + with patch( + "openjarvis.connectors.granola.httpx.get", + return_value=_FakeResponse(401), + ): + with pytest.raises(GranolaKeyError): + connector.handle_callback("fake-key-12345") + + # The pre-existing credential must be untouched. + stored = json.loads(creds_path.read_text()) + assert stored["token"] == "grl_real_existing_key" diff --git a/tests/connectors/test_slack_connector.py b/tests/connectors/test_slack_connector.py index e87a235d..1c22869d 100644 --- a/tests/connectors/test_slack_connector.py +++ b/tests/connectors/test_slack_connector.py @@ -560,3 +560,63 @@ def test_sync_logs_per_type_channel_counts( assert "1 private channels" in summary assert "2 DMs" in summary assert "1 group DMs" in summary + + +# --------------------------------------------------------------------------- +# Test — handle_callback verifies the token with a live auth.test BEFORE it +# persists anything, so a syntactically-valid-but-dead token is rejected at +# connect time instead of overwriting a working credential on disk. +# --------------------------------------------------------------------------- + + +@patch("openjarvis.connectors.slack_connector._slack_api_auth_test") +def test_handle_callback_persists_after_auth_test_succeeds( + mock_auth, connector +) -> None: + """A valid xoxp- token is persisted only after auth.test returns ok.""" + mock_auth.return_value = _AUTH_TEST_RESPONSE + + connector.handle_callback("xoxp-valid-user-token") + + mock_auth.assert_called_once_with("xoxp-valid-user-token") + stored = json.loads(Path(connector._credentials_path).read_text()) + assert stored["token"] == "xoxp-valid-user-token" + + +@patch("openjarvis.connectors.slack_connector._slack_api_auth_test") +def test_handle_callback_rejects_when_auth_test_fails(mock_auth, connector) -> None: + """A well-formed token Slack rejects (auth.test not ok) is never written.""" + from openjarvis.connectors.slack_connector import SlackTokenError # noqa: PLC0415 + + mock_auth.return_value = {"ok": False, "error": "invalid_auth"} + + with pytest.raises(SlackTokenError) as excinfo: + connector.handle_callback("xoxp-revoked-token") + + assert "invalid_auth" in str(excinfo.value) + assert not Path(connector._credentials_path).exists() + + +@patch("openjarvis.connectors.slack_connector._slack_api_auth_test") +def test_handle_callback_skips_auth_test_for_bad_shape(mock_auth, connector) -> None: + """Shape validation short-circuits before any network call is made.""" + from openjarvis.connectors.slack_connector import SlackTokenError # noqa: PLC0415 + + with pytest.raises(SlackTokenError): + connector.handle_callback("xoxb-bot-token") + + mock_auth.assert_not_called() + assert not Path(connector._credentials_path).exists() + + +def test_handle_callback_xoxb_message_wording(connector) -> None: + """The xoxb- rejection carries the user-facing 'can't read DMs' guidance.""" + from openjarvis.connectors.slack_connector import SlackTokenError # noqa: PLC0415 + + with pytest.raises(SlackTokenError) as excinfo: + connector.handle_callback("xoxb-bot-token") + + assert str(excinfo.value) == ( + "Bot tokens (xoxb-) can't read DMs. " + "Use a User OAuth Token (xoxp-) instead." + ) diff --git a/tests/server/test_connectors_router.py b/tests/server/test_connectors_router.py index d5421d33..a736fa06 100644 --- a/tests/server/test_connectors_router.py +++ b/tests/server/test_connectors_router.py @@ -102,3 +102,60 @@ def test_trigger_sync(app, tmp_path: Path) -> None: data = resp.json() assert data["connector_id"] == "obsidian" assert data["status"] in {"started", "already_syncing"} + + +# --------------------------------------------------------------------------- +# Connect-time credential validation (GH #409): the /connect endpoint must +# reject invalid credentials with HTTP 400 and never persist (or overwrite) +# anything on disk when validation fails. +# --------------------------------------------------------------------------- + + +def test_connect_slack_bot_token_returns_400(app, tmp_path: Path) -> None: + """POST connect with an xoxb- token is rejected 400 and writes nothing.""" + from openjarvis.connectors.slack_connector import SlackConnector + from openjarvis.server.connectors_router import _instances + + creds = tmp_path / "slack.json" + _instances["slack"] = SlackConnector(credentials_path=str(creds)) + try: + resp = app.post( + "/v1/connectors/slack/connect", json={"token": "xoxb-fake-token"} + ) + assert resp.status_code == 400 + assert "xoxb" in resp.json()["detail"].lower() + assert not creds.exists() + finally: + _instances.pop("slack", None) + + +def test_connect_granola_invalid_key_returns_400_keeps_existing( + app, tmp_path: Path +) -> None: + """A bad Granola key is rejected 400 and the existing credential survives.""" + import json + from unittest.mock import patch + + from openjarvis.connectors.granola import GranolaConnector, GranolaKeyError + from openjarvis.server.connectors_router import _instances + + creds = tmp_path / "granola.json" + creds.write_text(json.dumps({"token": "grl_real_existing_key"})) + _instances["granola"] = GranolaConnector(credentials_path=str(creds)) + try: + with patch( + "openjarvis.connectors.granola._granola_api_validate_key", + side_effect=GranolaKeyError( + "Invalid API key. Check your key in Granola Settings → API." + ), + ): + resp = app.post( + "/v1/connectors/granola/connect", + json={"code": "fake-key-12345"}, + ) + assert resp.status_code == 400 + assert "Invalid API key" in resp.json()["detail"] + # The previously-working credential must be untouched. + assert json.loads(creds.read_text())["token"] == "grl_real_existing_key" + finally: + _instances.pop("granola", None)