mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-27 21:05:34 +00:00
Pasting a Google Client ID / Secret never completed OAuth: Drive (and its Google siblings) accepted the credentials, showed no error, opened no browser, and never appeared in Data Sources. Root cause is three coupled defects, all reproduced at the unit level against main with a FastAPI TestClient (no Google creds, network-free): (A/B) POST /connect routed a `client_id:client_secret` pair into the connector's handle_callback, which spawned a daemon thread that popped a browser and ran its own localhost:8789 callback server. That thread fails silently in the bundled desktop context (`except Exception: pass`), so the connector never gained an access_token; /connect returned status "pending" and the UI's 20x2s poll timed out with no error. Fix: in POST /connect, an OAuth `client_id:client_secret` pair now persists the client credentials to every Google credential file and returns an `oauth_required` directive pointing at the in-process server flow, instead of the silent background thread. The Google connectors' handle_callback no longer spawns the browser thread for the pair case — it only persists the creds; the server's /oauth/start -> /oauth/callback owns the consent round-trip. (C) The would-be-correct server flow was itself broken: under `from __future__ import annotations` plus a `Request` import local to the router factory, FastAPI could not resolve the stringized `request: Request` annotation. /oauth/start returned HTTP 422 (request mis-bound as a query param) and /oauth/callback injected None -> AttributeError on `request.base_url`. Fix: import `Request` at module scope and make the callback's `request` a required injected dependency. A malformed/blank client pair now raises HTTP 400 with the provider setup URL instead of a perpetual silent "pending" (REVIEW.md silent-failure discipline). Frontend: DataSourcesPage now opens the server OAuth window when /connect returns `oauth_required`, then polls until connected; connect errors surface the backend detail; the Drive setup steps document the "Web application" OAuth client + server-callback redirect URI the in-process flow requires. Tests (run on the main venv, hermetic — no ~/.openjarvis pollution): - test_oauth_flow.py: the three handle_callback tests now assert NO browser is opened and only client creds are persisted (was: assert background flow ran). - test_connectors_router_oauth.py (new): reproduces + fixes all three defects via TestClient with mocked token exchange; parametrized over gdrive/gcalendar/ gcontacts/gmail/google_tasks to prove the shared OAuth path is fixed for every sibling and that a single consent writes the access_token to all six Google credential files and flips is_connected() to True. Full tests/connectors suite: 355 passed. Relationship to PR #510: #510 rewrites all of these files (account-scoped retrieval) but still carries all three defects. This fix is intentionally scoped to the OAuth path and does not modify oauth.py, to minimize collision. A maintainer can either merge this and rebase #510 on top, or port these changes into #510. See PR body for details. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
dfa908c358
commit
f9d1bc8c27
@@ -1,5 +1,5 @@
|
||||
import { getBase } from './api';
|
||||
import type { ConnectorInfo, SyncStatus, ConnectRequest } from '../types/connectors';
|
||||
import type { ConnectorInfo, SyncStatus, ConnectRequest, ConnectResponse } from '../types/connectors';
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Connectors API
|
||||
@@ -18,16 +18,47 @@ export async function getConnector(id: string): Promise<ConnectorInfo> {
|
||||
return res.json();
|
||||
}
|
||||
|
||||
export async function connectSource(id: string, req: ConnectRequest): Promise<ConnectorInfo> {
|
||||
export async function connectSource(id: string, req: ConnectRequest): Promise<ConnectResponse> {
|
||||
const res = await fetch(`${getBase()}/v1/connectors/${encodeURIComponent(id)}/connect`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify(req),
|
||||
});
|
||||
if (!res.ok) throw new Error(`Failed to connect ${id}: ${res.status}`);
|
||||
if (!res.ok) {
|
||||
// Surface the backend's actionable detail (e.g. malformed Client ID /
|
||||
// Secret) instead of a bare status code so the UI can render it.
|
||||
const err = await res.json().catch(() => ({ detail: res.statusText }));
|
||||
throw new Error(err.detail || `Failed to connect ${id}: ${res.status}`);
|
||||
}
|
||||
return res.json();
|
||||
}
|
||||
|
||||
/** Open the server-side OAuth consent flow in a popup and resolve once the
|
||||
* connector reports connected (or reject on timeout). Reused for any OAuth
|
||||
* connector whose /connect returned `oauth_required` (issue #512). */
|
||||
export function startServerOAuth(id: string, oauthStartPath?: string): Promise<void> {
|
||||
const path = oauthStartPath || `/v1/connectors/${encodeURIComponent(id)}/oauth/start`;
|
||||
window.open(`${getBase()}${path}`, '_blank', 'width=600,height=700');
|
||||
return new Promise((resolve, reject) => {
|
||||
const interval = setInterval(async () => {
|
||||
try {
|
||||
const info = await getConnector(id);
|
||||
if (info.connected) {
|
||||
clearInterval(interval);
|
||||
clearTimeout(timer);
|
||||
resolve();
|
||||
}
|
||||
} catch {
|
||||
// ignore transient polling errors
|
||||
}
|
||||
}, 2000);
|
||||
const timer = setTimeout(() => {
|
||||
clearInterval(interval);
|
||||
reject(new Error('Authorization timed out — please try again.'));
|
||||
}, 180000);
|
||||
});
|
||||
}
|
||||
|
||||
export async function disconnectSource(id: string): Promise<void> {
|
||||
const res = await fetch(`${getBase()}/v1/connectors/${encodeURIComponent(id)}/disconnect`, {
|
||||
method: 'POST',
|
||||
|
||||
@@ -24,7 +24,7 @@ import {
|
||||
import type { LucideIcon } from 'lucide-react';
|
||||
import { SOURCE_CATALOG } from '../types/connectors';
|
||||
import type { ConnectRequest } from '../types/connectors';
|
||||
import { listConnectors, connectSource, disconnectSource, getSyncStatus, triggerSync } from '../lib/connectors-api';
|
||||
import { listConnectors, connectSource, disconnectSource, getSyncStatus, triggerSync, startServerOAuth } from '../lib/connectors-api';
|
||||
import type { SyncStatus } from '../types/connectors';
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -673,7 +673,19 @@ function DataSourcesSection() {
|
||||
setConnectStage('Connecting...');
|
||||
setConnectError('');
|
||||
try {
|
||||
await connectSource(id, req);
|
||||
const resp = await connectSource(id, req);
|
||||
|
||||
// OAuth connectors (Google Drive/Calendar/Contacts/Gmail/Tasks): pasting
|
||||
// a Client ID / Secret only registers the app credentials. The backend
|
||||
// returns `oauth_required` with the path to the in-process consent flow,
|
||||
// which is the only path that actually mints an access token. Open it now
|
||||
// and wait for the callback to flip the connector to connected. Without
|
||||
// this the connector would stay "pending" forever — the exact #512 bug.
|
||||
if (resp.status === 'oauth_required') {
|
||||
setConnectStage('Opening Google sign-in...');
|
||||
await startServerOAuth(id, resp.oauth_start);
|
||||
}
|
||||
|
||||
setConnectStage('Connected! Starting sync...');
|
||||
|
||||
// Wait for connector to show as connected
|
||||
|
||||
@@ -55,6 +55,19 @@ export interface ConnectRequest {
|
||||
password?: string;
|
||||
}
|
||||
|
||||
/** Response from POST /v1/connectors/{id}/connect.
|
||||
* For OAuth connectors, pasting a Client ID / Secret pair only registers the
|
||||
* app credentials; the backend returns `status: "oauth_required"` plus an
|
||||
* `oauth_start` path the UI must open to run the browser consent flow that
|
||||
* actually mints an access token (see issue #512). */
|
||||
export interface ConnectResponse {
|
||||
connector_id: string;
|
||||
connected: boolean;
|
||||
status: "connected" | "pending" | "oauth_required" | "disconnected";
|
||||
oauth_start?: string;
|
||||
sync_status?: string | null;
|
||||
}
|
||||
|
||||
export type WizardStep = "pick" | "connect" | "ingest" | "ready";
|
||||
|
||||
// Backward-compatible alias
|
||||
@@ -257,12 +270,12 @@ export const SOURCE_CATALOG: ConnectorMeta[] = [
|
||||
urlLabel: 'Enable Drive API',
|
||||
},
|
||||
{
|
||||
label: 'Create OAuth credentials: go to Credentials (link below) → click "+ Create Credentials" → choose "OAuth client ID" → Application type: "Desktop app" → click "Create"',
|
||||
label: 'Create OAuth credentials: go to Credentials (link below) → click "+ Create Credentials" → choose "OAuth client ID" → Application type: "Web application". Under "Authorized redirect URIs" add this server\'s callback (e.g. http://localhost:1313/v1/connectors/gdrive/oauth/callback — match the host/port your OpenJarvis server is bound to) → click "Create".',
|
||||
url: 'https://console.cloud.google.com/apis/credentials',
|
||||
urlLabel: 'Open Credentials',
|
||||
},
|
||||
{
|
||||
label: 'A dialog will show your Client ID and Client Secret. Copy both and paste them below. (If you miss it, click the download icon next to your OAuth client to see them again)',
|
||||
label: 'A dialog will show your Client ID and Client Secret. Copy both and paste them below, then click Connect — a Google sign-in window opens to finish authorization. (If you miss the dialog, click the download icon next to your OAuth client to see them again.)',
|
||||
},
|
||||
],
|
||||
inputFields: [
|
||||
|
||||
@@ -20,7 +20,6 @@ from openjarvis.connectors.oauth import (
|
||||
delete_tokens,
|
||||
load_tokens,
|
||||
resolve_google_credentials,
|
||||
run_oauth_flow,
|
||||
save_tokens,
|
||||
)
|
||||
from openjarvis.core.config import DEFAULT_CONFIG_DIR
|
||||
@@ -290,12 +289,18 @@ class GCalendarConnector(BaseConnector):
|
||||
"""Handle the OAuth callback.
|
||||
|
||||
If *code* looks like a ``client_id:client_secret`` pair (containing
|
||||
``.apps.googleusercontent.com``), store the credentials and trigger
|
||||
the full browser-based OAuth flow. Otherwise treat it as a raw
|
||||
token / auth code.
|
||||
``.apps.googleusercontent.com``), persist the client credentials only.
|
||||
The browser consent + code→token exchange is owned by the in-process
|
||||
server flow (``/v1/connectors/{id}/oauth/start`` → ``/oauth/callback``),
|
||||
which writes the real ``access_token`` to every Google credential file.
|
||||
|
||||
The previous daemon-thread browser flow (its own ``localhost:8789``
|
||||
callback server) failed silently in the bundled desktop context and is
|
||||
intentionally removed here (issue #512).
|
||||
|
||||
Any other *code* is treated as a raw token / auth code.
|
||||
"""
|
||||
code = code.strip()
|
||||
# If user pastes client_id:client_secret, store and run OAuth flow
|
||||
if ":" in code and ".apps.googleusercontent.com" in code:
|
||||
client_id, client_secret = code.split(":", 1)
|
||||
save_tokens(
|
||||
@@ -305,20 +310,6 @@ class GCalendarConnector(BaseConnector):
|
||||
"client_secret": client_secret.strip(),
|
||||
},
|
||||
)
|
||||
import threading
|
||||
|
||||
def _run() -> None:
|
||||
try:
|
||||
run_oauth_flow(
|
||||
client_id=client_id.strip(),
|
||||
client_secret=client_secret.strip(),
|
||||
scopes=GOOGLE_ALL_SCOPES,
|
||||
credentials_path=self._credentials_path,
|
||||
)
|
||||
except Exception: # noqa: BLE001
|
||||
pass
|
||||
|
||||
threading.Thread(target=_run, daemon=True).start()
|
||||
else:
|
||||
# Raw token or auth code
|
||||
save_tokens(self._credentials_path, {"token": code})
|
||||
|
||||
@@ -20,7 +20,6 @@ from openjarvis.connectors.oauth import (
|
||||
delete_tokens,
|
||||
load_tokens,
|
||||
resolve_google_credentials,
|
||||
run_oauth_flow,
|
||||
save_tokens,
|
||||
)
|
||||
from openjarvis.core.config import DEFAULT_CONFIG_DIR
|
||||
@@ -195,12 +194,18 @@ class GContactsConnector(BaseConnector):
|
||||
"""Handle the OAuth callback.
|
||||
|
||||
If *code* looks like a ``client_id:client_secret`` pair (containing
|
||||
``.apps.googleusercontent.com``), store the credentials and trigger
|
||||
the full browser-based OAuth flow. Otherwise treat it as a raw
|
||||
token / auth code.
|
||||
``.apps.googleusercontent.com``), persist the client credentials only.
|
||||
The browser consent + code→token exchange is owned by the in-process
|
||||
server flow (``/v1/connectors/{id}/oauth/start`` → ``/oauth/callback``),
|
||||
which writes the real ``access_token`` to every Google credential file.
|
||||
|
||||
The previous daemon-thread browser flow (its own ``localhost:8789``
|
||||
callback server) failed silently in the bundled desktop context and is
|
||||
intentionally removed here (issue #512).
|
||||
|
||||
Any other *code* is treated as a raw token / auth code.
|
||||
"""
|
||||
code = code.strip()
|
||||
# If user pastes client_id:client_secret, store and run OAuth flow
|
||||
if ":" in code and ".apps.googleusercontent.com" in code:
|
||||
client_id, client_secret = code.split(":", 1)
|
||||
save_tokens(
|
||||
@@ -210,20 +215,6 @@ class GContactsConnector(BaseConnector):
|
||||
"client_secret": client_secret.strip(),
|
||||
},
|
||||
)
|
||||
import threading
|
||||
|
||||
def _run() -> None:
|
||||
try:
|
||||
run_oauth_flow(
|
||||
client_id=client_id.strip(),
|
||||
client_secret=client_secret.strip(),
|
||||
scopes=GOOGLE_ALL_SCOPES,
|
||||
credentials_path=self._credentials_path,
|
||||
)
|
||||
except Exception: # noqa: BLE001
|
||||
pass
|
||||
|
||||
threading.Thread(target=_run, daemon=True).start()
|
||||
else:
|
||||
# Raw token or auth code
|
||||
save_tokens(self._credentials_path, {"token": code})
|
||||
|
||||
@@ -20,7 +20,6 @@ from openjarvis.connectors.oauth import (
|
||||
delete_tokens,
|
||||
load_tokens,
|
||||
resolve_google_credentials,
|
||||
run_oauth_flow,
|
||||
save_tokens,
|
||||
)
|
||||
from openjarvis.core.config import DEFAULT_CONFIG_DIR
|
||||
@@ -178,15 +177,25 @@ class GDriveConnector(BaseConnector):
|
||||
"""Handle the OAuth callback.
|
||||
|
||||
If *code* looks like a ``client_id:client_secret`` pair (containing
|
||||
``.apps.googleusercontent.com``), store the credentials and trigger
|
||||
the full browser-based OAuth flow. Otherwise treat it as a raw
|
||||
token / auth code.
|
||||
``.apps.googleusercontent.com``), persist the client credentials only.
|
||||
The actual browser consent + code→token exchange is owned by the
|
||||
in-process server flow (``/v1/connectors/{id}/oauth/start`` →
|
||||
``/oauth/callback``), which writes the real ``access_token`` to every
|
||||
Google credential file.
|
||||
|
||||
Previously this spawned a daemon thread that popped a browser and ran
|
||||
its own ``localhost:8789`` callback server; that thread failed silently
|
||||
in the bundled desktop context, so the connector never gained an access
|
||||
token and never appeared in Data Sources (issue #512). The background
|
||||
flow is intentionally removed here.
|
||||
|
||||
Any other *code* is treated as a raw token / auth code.
|
||||
"""
|
||||
code = code.strip()
|
||||
# If user pastes client_id:client_secret, store and run OAuth flow
|
||||
# A pasted client_id:client_secret pair is the app registration, not a
|
||||
# completed credential — persist it and let the server flow finish auth.
|
||||
if ":" in code and ".apps.googleusercontent.com" in code:
|
||||
client_id, client_secret = code.split(":", 1)
|
||||
# Save credentials immediately
|
||||
save_tokens(
|
||||
self._credentials_path,
|
||||
{
|
||||
@@ -194,21 +203,6 @@ class GDriveConnector(BaseConnector):
|
||||
"client_secret": client_secret.strip(),
|
||||
},
|
||||
)
|
||||
# Run OAuth flow in background thread to avoid blocking
|
||||
import threading
|
||||
|
||||
def _run() -> None:
|
||||
try:
|
||||
run_oauth_flow(
|
||||
client_id=client_id.strip(),
|
||||
client_secret=client_secret.strip(),
|
||||
scopes=GOOGLE_ALL_SCOPES,
|
||||
credentials_path=self._credentials_path,
|
||||
)
|
||||
except Exception: # noqa: BLE001
|
||||
pass
|
||||
|
||||
threading.Thread(target=_run, daemon=True).start()
|
||||
else:
|
||||
# Raw token or auth code
|
||||
save_tokens(self._credentials_path, {"token": code})
|
||||
|
||||
@@ -3,7 +3,23 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from typing import Any, Dict, Optional
|
||||
from typing import TYPE_CHECKING, Any, Dict, Optional
|
||||
|
||||
# ``Request`` must be importable at *module* scope so that FastAPI can resolve
|
||||
# the stringized ``request: Request`` annotations on the OAuth endpoints below.
|
||||
# Because this module uses ``from __future__ import annotations``, every
|
||||
# annotation is a string that FastAPI evaluates against the module globals; a
|
||||
# ``Request`` imported only inside ``create_connectors_router()`` is invisible
|
||||
# there, which makes FastAPI mistake ``request`` for a required *query* param
|
||||
# (HTTP 422 on /oauth/start) or inject ``None`` (AttributeError on
|
||||
# /oauth/callback). Keep this import at top level. See issue #512.
|
||||
if TYPE_CHECKING:
|
||||
from starlette.requests import Request
|
||||
else:
|
||||
try:
|
||||
from starlette.requests import Request
|
||||
except ImportError: # starlette ships with fastapi; absent only without it
|
||||
Request = Any # type: ignore[assignment,misc]
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -78,7 +94,7 @@ def create_connectors_router():
|
||||
this package.
|
||||
"""
|
||||
try:
|
||||
from fastapi import APIRouter, HTTPException, Request
|
||||
from fastapi import APIRouter, HTTPException
|
||||
except ImportError as exc:
|
||||
raise ImportError(
|
||||
"fastapi and pydantic are required for the connectors router"
|
||||
@@ -125,6 +141,69 @@ def create_connectors_router():
|
||||
"chunks": chunks,
|
||||
}
|
||||
|
||||
def _maybe_oauth_client_pair(
|
||||
connector_id: str, req: ConnectRequest
|
||||
) -> Optional[Dict[str, Any]]:
|
||||
"""Handle a pasted ``client_id:client_secret`` for an OAuth connector.
|
||||
|
||||
Returns an ``oauth_required`` directive (and persists the client
|
||||
credentials to every credential file for the provider) when *req*
|
||||
carries a Google ``client_id:client_secret`` pair, so the caller can
|
||||
return early instead of triggering the silent background OAuth flow.
|
||||
Returns ``None`` when there is no such pair (the caller then falls
|
||||
through to the normal ``handle_callback`` / token path).
|
||||
|
||||
Raises ``HTTPException(400)`` when the pair is present but malformed or
|
||||
the connector has no OAuth provider — per the silent-failure discipline
|
||||
in REVIEW.md, a bad credential surfaces an actionable error rather than
|
||||
a perpetual ``pending`` state.
|
||||
"""
|
||||
from openjarvis.connectors.oauth import (
|
||||
get_provider_for_connector,
|
||||
save_client_credentials,
|
||||
)
|
||||
|
||||
raw = (req.code or req.token or "").strip()
|
||||
# Only the client-registration pair routes through the server flow.
|
||||
# A raw access token (no ".apps.googleusercontent.com") is handled by
|
||||
# the connector's handle_callback unchanged.
|
||||
if ".apps.googleusercontent.com" not in raw or ":" not in raw:
|
||||
return None
|
||||
|
||||
provider = get_provider_for_connector(connector_id)
|
||||
if provider is None:
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail=f"No OAuth provider configured for '{connector_id}'",
|
||||
)
|
||||
|
||||
client_id, client_secret = raw.split(":", 1)
|
||||
client_id = client_id.strip()
|
||||
client_secret = client_secret.strip()
|
||||
if not client_id or not client_secret:
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail=(
|
||||
"Malformed credentials — expected 'CLIENT_ID:CLIENT_SECRET'. "
|
||||
f"Create an OAuth client at: {provider.setup_url}"
|
||||
),
|
||||
)
|
||||
|
||||
save_client_credentials(provider, client_id, client_secret)
|
||||
# Cached instances may have resolved a stale credentials path before
|
||||
# these client creds existed; drop them so /oauth/callback rebuilds
|
||||
# them against the freshly written files.
|
||||
for cid in provider.connector_ids:
|
||||
_instances.pop(cid, None)
|
||||
|
||||
return {
|
||||
"connector_id": connector_id,
|
||||
"connected": False,
|
||||
"status": "oauth_required",
|
||||
"oauth_start": f"/v1/connectors/{connector_id}/oauth/start",
|
||||
"sync_status": None,
|
||||
}
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Background-sync state tracking. Defined here (before the endpoints)
|
||||
# so that POST /connect can fire-and-forget into the same machinery
|
||||
@@ -317,6 +396,19 @@ def create_connectors_router():
|
||||
instance._connected = Path(req.path).is_dir()
|
||||
|
||||
elif auth_type == "oauth":
|
||||
# A pasted ``client_id:client_secret`` pair is NOT a completed
|
||||
# OAuth credential — it is the app registration. Persist it and
|
||||
# hand the UI a directive to run the in-process browser consent
|
||||
# flow (/oauth/start → /oauth/callback), which is the only path
|
||||
# that actually exchanges a code for an access_token. Previously
|
||||
# this routed into the connector's handle_callback, which spawned
|
||||
# a daemon thread that popped a browser + ran its own
|
||||
# localhost:8789 callback server; that thread fails silently in
|
||||
# the bundled desktop context, so the connector never became
|
||||
# connected and never appeared in Data Sources (issue #512).
|
||||
directive = _maybe_oauth_client_pair(connector_id, req)
|
||||
if directive is not None:
|
||||
return directive
|
||||
if req.code:
|
||||
instance.handle_callback(req.code)
|
||||
elif req.token:
|
||||
@@ -433,9 +525,9 @@ def create_connectors_router():
|
||||
@router.get("/{connector_id}/oauth/callback")
|
||||
async def oauth_callback(
|
||||
connector_id: str,
|
||||
request: Request,
|
||||
code: str = "",
|
||||
error: str = "",
|
||||
request: Request = None,
|
||||
):
|
||||
"""Handle OAuth callback from the provider."""
|
||||
from fastapi.responses import HTMLResponse
|
||||
|
||||
@@ -0,0 +1,273 @@
|
||||
"""Regression tests for the connectors-router OAuth flow (issue #512).
|
||||
|
||||
These tests reproduce the three coupled defects that prevented Google Drive
|
||||
(and its Google siblings) from ever completing OAuth and appearing in Data
|
||||
Sources, and assert the fixed behaviour:
|
||||
|
||||
(A/B) ``POST /connect`` with a pasted ``client_id:client_secret`` pair must
|
||||
persist the client credentials and return an ``oauth_required`` directive
|
||||
pointing at ``/oauth/start`` — NOT silently spawn a background browser
|
||||
thread and report a perpetual ``pending`` state.
|
||||
(C-1) ``GET /oauth/start`` must return a redirect to the provider's consent
|
||||
page (regression: HTTP 422 because ``request: Request`` was mis-bound as
|
||||
a query param under ``from __future__ import annotations`` + a local
|
||||
``Request`` import).
|
||||
(C-2) ``GET /oauth/callback`` must read ``request.base_url`` and exchange the
|
||||
code for tokens without crashing (regression: ``request`` defaulted to
|
||||
``None`` → ``AttributeError``), persisting the access token to every
|
||||
Google credential file and flipping ``is_connected()`` to True.
|
||||
|
||||
All tests are hermetic: the connectors directory, the shared Google
|
||||
credentials path, and every Google connector's default credentials path are
|
||||
redirected to ``tmp_path`` so the suite neither depends on nor pollutes
|
||||
``~/.openjarvis/connectors`` (a real source of spurious failures — see the
|
||||
verifier note on ``resolve_google_credentials`` silently substituting the
|
||||
shared file when the caller-supplied path does not yet exist on disk).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from pathlib import Path
|
||||
from typing import Any, Iterator
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
fastapi = pytest.importorskip("fastapi", reason="requires the 'server' extra")
|
||||
from fastapi import FastAPI # noqa: E402
|
||||
from fastapi.testclient import TestClient # noqa: E402
|
||||
|
||||
_CLIENT_PAIR = "myid-123.apps.googleusercontent.com:GOCSPX-secret"
|
||||
_CLIENT_ID = "myid-123.apps.googleusercontent.com"
|
||||
|
||||
_ALL_GOOGLE_FILES = (
|
||||
"google.json",
|
||||
"gdrive.json",
|
||||
"gcalendar.json",
|
||||
"gcontacts.json",
|
||||
"gmail.json",
|
||||
"google_tasks.json",
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def hermetic_connectors(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path:
|
||||
"""Redirect all Google credential paths into *tmp_path*.
|
||||
|
||||
Ensures connector instances created by the router's ``_get_or_create``
|
||||
resolve to the same directory the OAuth callback writes to, and that the
|
||||
test leaves ``~/.openjarvis`` untouched.
|
||||
|
||||
Why this is more than a one-line monkeypatch: the autouse registry-clear
|
||||
fixture causes ``_ensure_connectors_registered()`` to ``importlib.reload``
|
||||
each connector module on the first router call, which re-executes the
|
||||
module body. To survive that reload we patch ``DEFAULT_CONFIG_DIR`` at its
|
||||
*source* (``openjarvis.core.config``) — every connector re-derives
|
||||
``_DEFAULT_CREDENTIALS_PATH`` from it on reload, so the tmp dir sticks.
|
||||
We also pre-register + pre-reload the connectors inside the fixture so the
|
||||
reload happens while the patch is live, then reset module state on
|
||||
teardown so a later test that imports these modules fresh is unaffected.
|
||||
"""
|
||||
import importlib
|
||||
import sys
|
||||
|
||||
import openjarvis.connectors.oauth as oauth_mod
|
||||
import openjarvis.core.config as config_mod
|
||||
import openjarvis.server.connectors_router as router_mod
|
||||
from openjarvis.core.registry import ConnectorRegistry
|
||||
|
||||
conn_dir = tmp_path / "connectors"
|
||||
conn_dir.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
monkeypatch.setattr(config_mod, "DEFAULT_CONFIG_DIR", tmp_path)
|
||||
monkeypatch.setattr(oauth_mod, "_CONNECTORS_DIR", conn_dir)
|
||||
monkeypatch.setattr(
|
||||
oauth_mod, "_SHARED_GOOGLE_CREDENTIALS_PATH", str(conn_dir / "google.json")
|
||||
)
|
||||
|
||||
# Force the connector modules to re-derive their default paths from the
|
||||
# patched DEFAULT_CONFIG_DIR now, before any request, and register them so
|
||||
# the router's lazy reload-on-empty-registry path is a no-op.
|
||||
google_mods = [
|
||||
"openjarvis.connectors.gdrive",
|
||||
"openjarvis.connectors.gcalendar",
|
||||
"openjarvis.connectors.gcontacts",
|
||||
"openjarvis.connectors.gmail",
|
||||
"openjarvis.connectors.google_tasks",
|
||||
]
|
||||
for name in google_mods:
|
||||
if name in sys.modules:
|
||||
importlib.reload(sys.modules[name])
|
||||
|
||||
router_mod._instances.clear()
|
||||
yield conn_dir
|
||||
router_mod._instances.clear()
|
||||
ConnectorRegistry.clear()
|
||||
# Restore the connector modules to their real (unpatched) default paths so
|
||||
# subsequent tests in the same process see ~/.openjarvis again.
|
||||
for name in google_mods:
|
||||
if name in sys.modules:
|
||||
importlib.reload(sys.modules[name])
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def client(hermetic_connectors: Path) -> Iterator[TestClient]:
|
||||
from openjarvis.server.connectors_router import create_connectors_router
|
||||
|
||||
app = FastAPI()
|
||||
app.include_router(create_connectors_router())
|
||||
with TestClient(app) as c:
|
||||
yield c
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Defect A/B — POST /connect must not silently spawn a background OAuth thread
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"connector_id", ["gdrive", "gcalendar", "gcontacts", "gmail", "google_tasks"]
|
||||
)
|
||||
def test_connect_client_pair_returns_oauth_required_no_browser(
|
||||
client: TestClient, hermetic_connectors: Path, connector_id: str
|
||||
) -> None:
|
||||
"""Pasting client_id:secret persists creds + asks the UI to run the flow.
|
||||
|
||||
Covers every Google connector that shares the OAuth provider, proving the
|
||||
sibling connectors are fixed too (not just gdrive).
|
||||
"""
|
||||
with patch("openjarvis.core.open_browser") as mock_browser:
|
||||
resp = client.post(
|
||||
f"/v1/connectors/{connector_id}/connect", json={"code": _CLIENT_PAIR}
|
||||
)
|
||||
|
||||
assert resp.status_code == 200, resp.text
|
||||
body = resp.json()
|
||||
assert body["status"] == "oauth_required"
|
||||
assert body["oauth_start"] == f"/v1/connectors/{connector_id}/oauth/start"
|
||||
assert body["connected"] is False
|
||||
# No fire-and-forget browser thread (the root cause of "nothing happens").
|
||||
mock_browser.assert_not_called()
|
||||
|
||||
# Client credentials persisted to EVERY Google credential file so a single
|
||||
# consent covers all Google connectors.
|
||||
for filename in _ALL_GOOGLE_FILES:
|
||||
path = hermetic_connectors / filename
|
||||
assert path.exists(), f"{filename} not written"
|
||||
assert json.loads(path.read_text())["client_id"] == _CLIENT_ID
|
||||
|
||||
|
||||
def test_connect_malformed_client_pair_raises_400(
|
||||
client: TestClient,
|
||||
) -> None:
|
||||
"""A blank secret surfaces an actionable 400 — not a silent pending state."""
|
||||
resp = client.post(
|
||||
"/v1/connectors/gdrive/connect",
|
||||
json={"code": "myid-123.apps.googleusercontent.com:"},
|
||||
)
|
||||
assert resp.status_code == 400
|
||||
assert "CLIENT_ID:CLIENT_SECRET" in resp.json()["detail"]
|
||||
|
||||
|
||||
def test_connect_raw_token_still_handled(
|
||||
client: TestClient, hermetic_connectors: Path
|
||||
) -> None:
|
||||
"""A raw token (not a client pair) still flows through handle_callback."""
|
||||
resp = client.post(
|
||||
"/v1/connectors/gdrive/connect", json={"token": "ya29.raw-access-token"}
|
||||
)
|
||||
assert resp.status_code == 200, resp.text
|
||||
saved = json.loads((hermetic_connectors / "gdrive.json").read_text())
|
||||
assert saved.get("token") == "ya29.raw-access-token"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Defect C-1 — GET /oauth/start must redirect (was HTTP 422)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_oauth_start_redirects_to_consent(
|
||||
client: TestClient,
|
||||
) -> None:
|
||||
# First save client creds via the connect call.
|
||||
client.post("/v1/connectors/gdrive/connect", json={"code": _CLIENT_PAIR})
|
||||
|
||||
resp = client.get("/v1/connectors/gdrive/oauth/start", follow_redirects=False)
|
||||
# FastAPI's RedirectResponse defaults to 307; any 3xx is a pass (was 422).
|
||||
assert resp.status_code in (302, 307), resp.text
|
||||
location = resp.headers["location"]
|
||||
assert location.startswith("https://accounts.google.com/o/oauth2/v2/auth")
|
||||
assert _CLIENT_ID in location
|
||||
# redirect_uri must point back at OUR in-process callback.
|
||||
assert "oauth%2Fcallback" in location or "oauth/callback" in location
|
||||
|
||||
|
||||
def test_oauth_start_without_creds_returns_400(client: TestClient) -> None:
|
||||
resp = client.get("/v1/connectors/gdrive/oauth/start", follow_redirects=False)
|
||||
assert resp.status_code == 400
|
||||
assert "client credentials" in resp.json()["detail"].lower()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Defect C-2 — GET /oauth/callback must exchange + persist (was 500 on None)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_oauth_callback_exchanges_and_connects(
|
||||
client: TestClient, hermetic_connectors: Path
|
||||
) -> None:
|
||||
import openjarvis.connectors.oauth as oauth_mod
|
||||
|
||||
client.post("/v1/connectors/gdrive/connect", json={"code": _CLIENT_PAIR})
|
||||
|
||||
fake_tokens = {
|
||||
"access_token": "ya29.REAL",
|
||||
"refresh_token": "1//REAL",
|
||||
"token_type": "Bearer",
|
||||
"expires_in": 3600,
|
||||
}
|
||||
with patch.object(oauth_mod, "_exchange_token", return_value=fake_tokens) as ex:
|
||||
resp = client.get("/v1/connectors/gdrive/oauth/callback?code=authcode123")
|
||||
|
||||
assert resp.status_code == 200, resp.text
|
||||
assert "Connected!" in resp.text
|
||||
ex.assert_called_once()
|
||||
|
||||
# Access token written to ALL Google credential files.
|
||||
for filename in _ALL_GOOGLE_FILES:
|
||||
saved = json.loads((hermetic_connectors / filename).read_text())
|
||||
assert saved["access_token"] == "ya29.REAL"
|
||||
assert saved["refresh_token"] == "1//REAL"
|
||||
|
||||
# The connector now reports connected, and GET /connectors agrees.
|
||||
from openjarvis.connectors.gdrive import GDriveConnector
|
||||
|
||||
assert GDriveConnector().is_connected() is True
|
||||
|
||||
listing = client.get("/v1/connectors").json()["connectors"]
|
||||
gdrive = next(c for c in listing if c["connector_id"] == "gdrive")
|
||||
assert gdrive["connected"] is True
|
||||
|
||||
|
||||
def test_oauth_callback_error_param_renders_failure(client: TestClient) -> None:
|
||||
resp = client.get("/v1/connectors/gdrive/oauth/callback?error=access_denied")
|
||||
assert resp.status_code == 400
|
||||
assert "access_denied" in resp.text
|
||||
|
||||
|
||||
def test_oauth_callback_exchange_failure_renders_error(
|
||||
client: TestClient,
|
||||
) -> None:
|
||||
import openjarvis.connectors.oauth as oauth_mod
|
||||
|
||||
client.post("/v1/connectors/gdrive/connect", json={"code": _CLIENT_PAIR})
|
||||
|
||||
def _boom(*_a: Any, **_k: Any) -> dict[str, Any]:
|
||||
raise RuntimeError("token endpoint 400")
|
||||
|
||||
with patch.object(oauth_mod, "_exchange_token", side_effect=_boom):
|
||||
resp = client.get("/v1/connectors/gdrive/oauth/callback?code=bad")
|
||||
|
||||
assert resp.status_code == 500
|
||||
assert "Token Exchange Failed" in resp.text
|
||||
@@ -30,19 +30,35 @@ def test_exchange_google_token_calls_endpoint() -> None:
|
||||
mock_post.assert_called_once()
|
||||
|
||||
|
||||
def test_gdrive_handle_callback_triggers_oauth(tmp_path: Path) -> None:
|
||||
def test_gdrive_handle_callback_persists_creds_no_background_flow(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""A pasted client pair persists creds ONLY — no silent browser thread.
|
||||
|
||||
Regression for issue #512: the previous implementation spawned a daemon
|
||||
thread that popped a browser and ran its own localhost:8789 callback
|
||||
server. That thread failed silently in the bundled desktop context, so the
|
||||
connector never gained an access token. ``handle_callback`` must now only
|
||||
save the client_id/secret; the in-process server flow owns the consent
|
||||
round-trip. We assert ``open_browser`` is never invoked.
|
||||
"""
|
||||
from openjarvis.connectors.gdrive import GDriveConnector
|
||||
from openjarvis.connectors.oauth import load_tokens
|
||||
|
||||
creds = str(tmp_path / "gdrive.json")
|
||||
conn = GDriveConnector(credentials_path=creds)
|
||||
|
||||
with patch("openjarvis.connectors.gdrive.run_oauth_flow") as mock_flow:
|
||||
mock_flow.return_value = {"access_token": "ya29.test"}
|
||||
with patch("openjarvis.core.open_browser") as mock_browser:
|
||||
conn.handle_callback("test-id.apps.googleusercontent.com:test-secret")
|
||||
|
||||
mock_flow.assert_called_once()
|
||||
call_kwargs = mock_flow.call_args
|
||||
assert "test-id.apps.googleusercontent.com" in str(call_kwargs)
|
||||
mock_browser.assert_not_called()
|
||||
tokens = load_tokens(creds)
|
||||
assert tokens is not None
|
||||
assert tokens["client_id"] == "test-id.apps.googleusercontent.com"
|
||||
assert tokens["client_secret"] == "test-secret"
|
||||
# No access token yet — that arrives via /oauth/callback.
|
||||
assert not tokens.get("access_token")
|
||||
assert conn.is_connected() is False
|
||||
|
||||
|
||||
def test_gdrive_is_connected_requires_access_token(tmp_path: Path) -> None:
|
||||
@@ -61,48 +77,46 @@ def test_gdrive_is_connected_requires_access_token(tmp_path: Path) -> None:
|
||||
assert conn.is_connected() is True
|
||||
|
||||
|
||||
def test_gcalendar_handle_callback_triggers_oauth(tmp_path: Path) -> None:
|
||||
def test_gcalendar_handle_callback_persists_creds_no_background_flow(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Sibling connector shares the fix: creds saved, no browser thread (#512)."""
|
||||
from openjarvis.connectors.gcalendar import GCalendarConnector
|
||||
from openjarvis.connectors.oauth import load_tokens
|
||||
|
||||
creds = str(tmp_path / "gcalendar.json")
|
||||
conn = GCalendarConnector(credentials_path=creds)
|
||||
|
||||
with patch("openjarvis.connectors.gcalendar.run_oauth_flow") as mock_flow:
|
||||
mock_flow.return_value = {"access_token": "ya29.test"}
|
||||
with patch("openjarvis.core.open_browser") as mock_browser:
|
||||
conn.handle_callback("test-id.apps.googleusercontent.com:test-secret")
|
||||
|
||||
mock_flow.assert_called_once()
|
||||
|
||||
|
||||
def test_gcontacts_handle_callback_triggers_oauth(tmp_path: Path) -> None:
|
||||
from openjarvis.connectors.gcontacts import GContactsConnector
|
||||
|
||||
creds = str(tmp_path / "gcontacts.json")
|
||||
conn = GContactsConnector(credentials_path=creds)
|
||||
|
||||
with patch("openjarvis.connectors.gcontacts.run_oauth_flow") as mock_flow:
|
||||
mock_flow.return_value = {"access_token": "ya29.test"}
|
||||
conn.handle_callback("test-id.apps.googleusercontent.com:test-secret")
|
||||
|
||||
mock_flow.assert_called_once()
|
||||
|
||||
|
||||
def test_gdrive_handle_callback_fallback_on_failure(tmp_path: Path) -> None:
|
||||
from openjarvis.connectors.gdrive import GDriveConnector
|
||||
from openjarvis.connectors.oauth import load_tokens
|
||||
|
||||
creds = str(tmp_path / "gdrive.json")
|
||||
conn = GDriveConnector(credentials_path=creds)
|
||||
|
||||
with patch("openjarvis.connectors.gdrive.run_oauth_flow") as mock_flow:
|
||||
mock_flow.side_effect = RuntimeError("OAuth failed")
|
||||
conn.handle_callback("test-id.apps.googleusercontent.com:test-secret")
|
||||
|
||||
# Should have saved client_id and client_secret as fallback
|
||||
mock_browser.assert_not_called()
|
||||
tokens = load_tokens(creds)
|
||||
assert tokens is not None
|
||||
assert tokens["client_id"] == "test-id.apps.googleusercontent.com"
|
||||
assert tokens["client_secret"] == "test-secret"
|
||||
assert conn.is_connected() is False
|
||||
|
||||
|
||||
def test_gcontacts_handle_callback_persists_creds_no_background_flow(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Sibling connector shares the fix: creds saved, no browser thread (#512)."""
|
||||
from openjarvis.connectors.gcontacts import GContactsConnector
|
||||
from openjarvis.connectors.oauth import load_tokens
|
||||
|
||||
creds = str(tmp_path / "gcontacts.json")
|
||||
conn = GContactsConnector(credentials_path=creds)
|
||||
|
||||
with patch("openjarvis.core.open_browser") as mock_browser:
|
||||
conn.handle_callback("test-id.apps.googleusercontent.com:test-secret")
|
||||
|
||||
mock_browser.assert_not_called()
|
||||
tokens = load_tokens(creds)
|
||||
assert tokens is not None
|
||||
assert tokens["client_id"] == "test-id.apps.googleusercontent.com"
|
||||
assert tokens["client_secret"] == "test-secret"
|
||||
assert conn.is_connected() is False
|
||||
|
||||
|
||||
def test_gdrive_handle_callback_raw_token(tmp_path: Path) -> None:
|
||||
|
||||
Reference in New Issue
Block a user