diff --git a/.github/workflows/pr-desktop-build.yml b/.github/workflows/pr-desktop-build.yml index b72d800db34..7df91a38bc1 100644 --- a/.github/workflows/pr-desktop-build.yml +++ b/.github/workflows/pr-desktop-build.yml @@ -25,7 +25,10 @@ jobs: build-desktop: name: Build Desktop (${{ matrix.platform }}) runs-on: ${{ matrix.os }} - timeout-minutes: 60 + # Windows runners are the slow leg: a cold cache compiles the whole Tauri + # dep tree twice (Clippy, then the release build), incl. native aws-lc-sys. + # Matches the desktop build timeout in deployment.yml. + timeout-minutes: 90 strategy: fail-fast: false matrix: @@ -38,11 +41,10 @@ jobs: os: macos-latest target: universal-apple-darwin args: "--target universal-apple-darwin" - # TODO: Fix and enable the Windows build. - #- platform: windows - # os: windows-latest - # target: x86_64-pc-windows-msvc - # args: "" + - platform: windows + os: windows-latest + target: x86_64-pc-windows-msvc + args: "" steps: - name: Checkout code @@ -106,6 +108,20 @@ jobs: working-directory: ./desktop/src-tauri run: cargo clippy --all-targets --all-features -- -D warnings + # `tauri build` bundles an MSI on Windows, and WiX's ProductVersion only + # accepts numeric `major.minor.patch[.build]` -- the checked-in + # `0.0.0-dev` fails with "optional pre-release identifier in app version + # must be numeric-only ... for msi target". The release workflow injects a + # real numeric version at this point; presubmit has none to inject, so + # pin a throwaway one to keep MSI bundling exercised on PRs. + - name: Pin an MSI-compatible version (Windows) + if: matrix.platform == 'windows' + shell: bash + working-directory: ./desktop/src-tauri + run: | + jq '.version = "0.0.0"' tauri.conf.json > tauri.conf.json.tmp + mv tauri.conf.json.tmp tauri.conf.json + - name: Build desktop app working-directory: ./desktop run: bunx tauri build ${{ matrix.args }} diff --git a/.greptile/rules.md b/.greptile/rules.md index 1eeb71f8592..c5b13d24016 100644 --- a/.greptile/rules.md +++ b/.greptile/rules.md @@ -24,16 +24,18 @@ When hardcoding a boolean variable to a constant value, remove the variable enti Code changes must consider both multi-tenant and single-tenant deployments. In multi-tenant mode, preserve tenant isolation, ensure tenant context is propagated correctly, and avoid assumptions that only hold for a single shared schema or globally shared state. In single-tenant mode, avoid introducing unnecessary tenant-specific requirements or cloud-only control-plane dependencies. -## Nginx Routing — New Backend Routes +## Routing for New Non-/api Backend Routes -Whenever a new backend route is added that does NOT start with `/api`, it must also be explicitly added to ALL nginx configs: +Whenever a new backend route is added that does NOT start with `/api`, it must be explicitly routed in ALL nginx configs: -- `deployment/helm/charts/onyx/templates/nginx-conf.yaml` (Helm/k8s) +- `deployment/helm/charts/onyx/templates/nginx-conf.yaml` (Helm/k8s, bundled nginx) - `deployment/data/nginx/app.conf.template` (docker-compose dev) - `deployment/data/nginx/app.conf.template.prod` (docker-compose prod) - `deployment/data/nginx/app.conf.template.no-letsencrypt` (docker-compose no-letsencrypt) -Routes not starting with `/api` are not caught by the existing `^/(api|openapi\.json)` location block and will fall through to `location /`, which proxies to the Next.js web server and returns an HTML 404. The new location block must be placed before the `/api` block. Examples of routes that need this treatment: `/scim`, `/mcp`. +Routes not starting with `/api` are not caught by the existing `^/(api|openapi\.json)` location block and will fall through to `location /`, which proxies to the Next.js web server and returns an HTML 404. In the nginx configs, the new location block must be placed before the `/api` block. Examples of routes that need this treatment: `/scim`, `/mcp`. + +The route must ALSO be covered in Helm ingress mode (`ingress.enabled=true`), which does not use the bundled nginx. Add a dedicated ingress template for the route (see `deployment/helm/charts/onyx/templates/ingress-scim.yaml`). `ingress-api.yaml` cannot host it, since that resource only routes `/api` and its resource-wide rewrite annotation strips other prefixes. If the web app owns a sub-path of the route (for example an IdP callback page), carve that sub-path back out to the webserver with `pathType: Exact`, following `ingress-mcp-oauth-callback.yaml`. ## Full vs Lite Deployments diff --git a/AGENTS.md b/AGENTS.md index 0b9675b374f..eb3b913e1da 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,8 +63,20 @@ pre-commit run --files [ ...] NOTE: Always make sure everything is strictly typed (both in Python and Typescript). -NOTE: Keep comments brief and focused on information that stays relevant long-term. Don't write -comments that only describe the instantaneous change (e.g. what was just added/removed/refactored). +NOTE: Keep code comments brief and focused on information that stays relevant long-term. + +## Writing + +These rules apply to all prose you write: docs, commit messages, PR descriptions, reports, and replies. + +Follow ASD-STE100 Simplified Technical English for technical text: + +- Use approved words only. Each word has one meaning. +- Use one word for one idea. Do not use two words for the same thing. +- Write short sentences. Use 20 words or less for instructions. +- Use active voice. Write "Turn the switch", not "The switch must be turned". +- Write short paragraphs. Keep one topic in each paragraph. +- Keep code comments focused on information that is relevant long-term or for future readers. ## Testing diff --git a/backend/onyx/external_apps/providers/actions.py b/backend/onyx/external_apps/providers/actions.py index 220f4383bbf..588200fae73 100644 --- a/backend/onyx/external_apps/providers/actions.py +++ b/backend/onyx/external_apps/providers/actions.py @@ -116,3 +116,6 @@ class EndpointSpec(BaseModel): # The policy a freshly-created built-in app starts this action at, unless the # admin overrides it. default_policy: EndpointPolicy = EndpointPolicy.ASK + # Set when the action needs a scope only a self-hosted deployment requests, + # which drops it from the cloud catalog (``registry.get_endpoint_catalog``). + requires_self_hosted_scope: bool = False diff --git a/backend/onyx/external_apps/providers/gmail.py b/backend/onyx/external_apps/providers/gmail.py index d23b1e2d7bb..b7bc6820220 100644 --- a/backend/onyx/external_apps/providers/gmail.py +++ b/backend/onyx/external_apps/providers/gmail.py @@ -16,6 +16,7 @@ ) from onyx.external_apps.providers.base import OnyxManagedExtApp from onyx.external_apps.providers.google_base import GoogleOAuthProvider +from shared_configs.configs import MULTI_TENANT # Gmail API v1 (https://gmail.googleapis.com/gmail/v1/users/{userId}/...); the @@ -56,6 +57,7 @@ class GmailAction(ExternalAppAction): RestRoute(method="GET", path=_MESSAGE_ITEM), ), default_policy=EndpointPolicy.ALWAYS, + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.LABELS_READ, @@ -63,6 +65,9 @@ class GmailAction(ExternalAppAction): description="List the labels in the mailbox.", matches=(RestRoute(method="GET", path=f"{_USER}/labels"),), default_policy=EndpointPolicy.ALWAYS, + # `gmail.labels` isn't restricted, but labels are only useful alongside + # the message reads that are. + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.PROFILE_READ, @@ -70,6 +75,7 @@ class GmailAction(ExternalAppAction): description="Read the connected account's Gmail profile.", matches=(RestRoute(method="GET", path=f"{_USER}/profile"),), default_policy=EndpointPolicy.ALWAYS, + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.MESSAGES_SEND, @@ -82,12 +88,14 @@ class GmailAction(ExternalAppAction): normalised_name="Modify message labels", description="Add or remove labels on a message (mark read, archive, …).", matches=(RestRoute(method="POST", path=f"{_MESSAGE_ITEM}/modify"),), + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.MESSAGES_TRASH, normalised_name="Trash a message", description="Move a message to the trash.", matches=(RestRoute(method="POST", path=f"{_MESSAGE_ITEM}/trash"),), + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.THREADS_READ, @@ -98,6 +106,7 @@ class GmailAction(ExternalAppAction): RestRoute(method="GET", path=_THREAD_ITEM), ), default_policy=EndpointPolicy.ALWAYS, + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.ATTACHMENTS_READ, @@ -109,6 +118,7 @@ class GmailAction(ExternalAppAction): ), ), default_policy=EndpointPolicy.ALWAYS, + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.DRAFTS_READ, @@ -119,6 +129,7 @@ class GmailAction(ExternalAppAction): RestRoute(method="GET", path=_DRAFT_ITEM), ), default_policy=EndpointPolicy.ALWAYS, + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.DRAFTS_CREATE, @@ -128,6 +139,7 @@ class GmailAction(ExternalAppAction): description="Save a new draft email (not sent).", matches=(RestRoute(method="POST", path=_DRAFTS),), default_policy=EndpointPolicy.ALWAYS, + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.DRAFTS_UPDATE, @@ -135,30 +147,39 @@ class GmailAction(ExternalAppAction): description="Replace the contents of an existing draft (not sent).", matches=(RestRoute(method="PUT", path=_DRAFT_ITEM),), default_policy=EndpointPolicy.ALWAYS, + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.DRAFTS_DELETE, normalised_name="Delete a draft", description="Permanently delete a draft.", matches=(RestRoute(method="DELETE", path=_DRAFT_ITEM),), + requires_self_hosted_scope=True, ), EndpointSpec( id=GmailAction.DRAFTS_SEND, normalised_name="Send a draft", description="Send an existing draft as an email.", matches=(RestRoute(method="POST", path=f"{_DRAFTS}/send"),), + requires_self_hosted_scope=True, ), ] +# gmail.modify covers read, send, label, trash, threads, attachments, and the +# full draft lifecycle — but not permanent message delete, which keeps the +# integration safer by default. +_SELF_HOSTED_SCOPE = "https://www.googleapis.com/auth/gmail.modify" +# Every Gmail scope that can read mail or touch drafts is restricted, so on +# cloud the app is send-only. +_CLOUD_SCOPE = "https://www.googleapis.com/auth/gmail.send" + + class GmailProvider(GoogleOAuthProvider, OnyxManagedExtApp): spec = GoogleOAuthProvider.build_spec( app_type=ExternalAppType.GMAIL, app_name="Gmail", - # gmail.modify covers read, send, label, trash, threads, attachments, and - # the full draft lifecycle — but not permanent message delete, which keeps - # the integration safer by default. - scope="https://www.googleapis.com/auth/gmail.modify", + scope=_CLOUD_SCOPE if MULTI_TENANT else _SELF_HOSTED_SCOPE, upstream_url_patterns=["https://gmail\\.googleapis\\.com/gmail/.*"], google_api_name="Gmail API", endpoint_catalog=_ENDPOINTS, diff --git a/backend/onyx/external_apps/providers/google_base.py b/backend/onyx/external_apps/providers/google_base.py index 35deb874da2..209d922e231 100644 --- a/backend/onyx/external_apps/providers/google_base.py +++ b/backend/onyx/external_apps/providers/google_base.py @@ -16,6 +16,12 @@ _AUTHORIZE_URL = "https://accounts.google.com/o/oauth2/v2/auth" _TOKEN_URL = "https://oauth2.googleapis.com/token" +# Google's restricted scopes (all of Gmail bar `gmail.send`/`gmail.labels`, all +# of Drive bar `drive.file`) put the OAuth client under an annual third-party +# security assessment. Cloud runs Onyx's verified client, so each provider picks +# a restricted-free scope there and marks the actions it can't cover +# `requires_self_hosted_scope`. + # Every Google provider authenticates with the same Cloud Console OAuth client. _CLIENT_CREDENTIAL_FIELDS = [ OrgCredentialField( diff --git a/backend/onyx/external_apps/providers/google_calendar.py b/backend/onyx/external_apps/providers/google_calendar.py index 24c5fe7e626..bc858ab9715 100644 --- a/backend/onyx/external_apps/providers/google_calendar.py +++ b/backend/onyx/external_apps/providers/google_calendar.py @@ -10,6 +10,7 @@ ) from onyx.external_apps.providers.base import OnyxManagedExtApp from onyx.external_apps.providers.google_base import GoogleOAuthProvider +from shared_configs.configs import MULTI_TENANT # Google Calendar REST v3 (https://www.googleapis.com/calendar/v3/...); the @@ -79,11 +80,21 @@ class GoogleCalendarAction(ExternalAppAction): ] +_SELF_HOSTED_SCOPE = "https://www.googleapis.com/auth/calendar" +# No Calendar scope is restricted, so this is just the least-privilege spelling +# of the above — the whole catalog survives. +_CLOUD_SCOPE = ( + "https://www.googleapis.com/auth/calendar.events " + "https://www.googleapis.com/auth/calendar.calendarlist.readonly " + "https://www.googleapis.com/auth/calendar.freebusy" +) + + class GoogleCalendarProvider(GoogleOAuthProvider, OnyxManagedExtApp): spec = GoogleOAuthProvider.build_spec( app_type=ExternalAppType.GOOGLE_CALENDAR, app_name="Google Calendar", - scope="https://www.googleapis.com/auth/calendar", + scope=_CLOUD_SCOPE if MULTI_TENANT else _SELF_HOSTED_SCOPE, upstream_url_patterns=["https://www\\.googleapis\\.com/calendar/.*"], google_api_name="Google Calendar API", endpoint_catalog=_ENDPOINTS, diff --git a/backend/onyx/external_apps/providers/google_drive.py b/backend/onyx/external_apps/providers/google_drive.py index 702a7e99295..d391c180e85 100644 --- a/backend/onyx/external_apps/providers/google_drive.py +++ b/backend/onyx/external_apps/providers/google_drive.py @@ -10,6 +10,7 @@ ) from onyx.external_apps.providers.base import OnyxManagedExtApp from onyx.external_apps.providers.google_base import GoogleOAuthProvider +from shared_configs.configs import MULTI_TENANT # Google Drive API v3. Reads (GET) live under `/drive/v3/...`; content uploads @@ -71,6 +72,8 @@ class GoogleDriveAction(ExternalAppAction): RestRoute(method="GET", path="/drive/v3/drives/{driveId}"), ), default_policy=EndpointPolicy.ALWAYS, + # drives.list needs a Drive-wide scope; `drive.file` can't reach it. + requires_self_hosted_scope=True, ), EndpointSpec( id=GoogleDriveAction.FILES_CREATE, @@ -124,13 +127,23 @@ class GoogleDriveAction(ExternalAppAction): ] +# Full drive scope: read, search, create, edit, and delete any of the user's +# files. Mutations are gated by per-action ASK approval. +_SELF_HOSTED_SCOPE = "https://www.googleapis.com/auth/drive" +# Every Drive-wide scope is restricted, so cloud pairs per-file access (files +# Onyx created or the user opened with it) with the Docs API, which reaches any +# Google Doc by id. +_CLOUD_SCOPE = ( + "https://www.googleapis.com/auth/drive.file " + "https://www.googleapis.com/auth/documents" +) + + class GoogleDriveProvider(GoogleOAuthProvider, OnyxManagedExtApp): spec = GoogleOAuthProvider.build_spec( app_type=ExternalAppType.GOOGLE_DRIVE, app_name="Google Drive", - # Full drive scope: read, search, create, edit, and delete any of the - # user's files. Mutations are gated by per-action ASK approval. - scope="https://www.googleapis.com/auth/drive", + scope=_CLOUD_SCOPE if MULTI_TENANT else _SELF_HOSTED_SCOPE, upstream_url_patterns=[ "https://www\\.googleapis\\.com/drive/.*", # Content uploads use the separate /upload host path. diff --git a/backend/onyx/external_apps/providers/registry.py b/backend/onyx/external_apps/providers/registry.py index 4b8d2c1cae5..a1665213e70 100644 --- a/backend/onyx/external_apps/providers/registry.py +++ b/backend/onyx/external_apps/providers/registry.py @@ -18,6 +18,7 @@ from onyx.external_apps.providers.linear import LinearProvider from onyx.external_apps.providers.notion import NotionProvider from onyx.external_apps.providers.slack import SlackProvider +from shared_configs.configs import MULTI_TENANT _PROVIDER_CLASSES: list[type[ExternalAppProvider]] = [ SlackProvider, @@ -61,6 +62,14 @@ def get_onyx_managed_provider(app_type: ExternalAppType) -> OnyxManagedExtApp | return provider if isinstance(provider, OnyxManagedExtApp) else None +def uses_cloud_scope(app_type: ExternalAppType) -> bool: + """Whether this app connects with Onyx's cloud OAuth client rather than + credentials the deployment owns. That client is verified with the upstream + provider, so providers narrow their scope there (see ``GoogleOAuthProvider``) + and the actions it can't cover drop out of the catalog.""" + return MULTI_TENANT and get_onyx_managed_provider(app_type) is not None + + def get_provider_or_raise(app: ExternalApp) -> ExternalAppProvider: provider = get_provider_for_app(app) if provider is None: @@ -98,15 +107,23 @@ def _descriptor_for( description=e.description, default_policy=e.default_policy, ) - for e in spec.endpoint_catalog + for e in get_endpoint_catalog(spec.app_type) ], ) def get_endpoint_catalog(app_type: ExternalAppType) -> list[EndpointSpec]: - """The action catalog for an app_type (empty for CUSTOM / unregistered).""" + """The action catalog for an app_type (empty for CUSTOM / unregistered), + minus the actions the OAuth grant in force can't cover. Every consumer + (admin view, policy resolution, the runtime gate) funnels through here, so + none of them can offer an action the grant won't authorize.""" provider = PROVIDERS.get(app_type) - return list(provider.spec.endpoint_catalog) if provider is not None else [] + if provider is None: + return [] + catalog = provider.spec.endpoint_catalog + if uses_cloud_scope(app_type): + return [e for e in catalog if not e.requires_self_hosted_scope] + return list(catalog) def effective_policy( diff --git a/backend/onyx/server/features/mcp/api.py b/backend/onyx/server/features/mcp/api.py index 0483019d2bc..55da69446cd 100644 --- a/backend/onyx/server/features/mcp/api.py +++ b/backend/onyx/server/features/mcp/api.py @@ -899,6 +899,7 @@ async def _connect_oauth( connection_headers=connection_config_dict.get("headers", {}), transport=mcp_server.transport, is_authenticated=is_authenticated, + force_reauthentication=request.force_reauthentication, ) except OnyxError: raise diff --git a/backend/onyx/server/features/mcp/models.py b/backend/onyx/server/features/mcp/models.py index d93a166eb19..7dd6a9520ad 100644 --- a/backend/onyx/server/features/mcp/models.py +++ b/backend/onyx/server/features/mcp/models.py @@ -486,6 +486,10 @@ class MCPUserOAuthConnectRequest(BaseModel): server_id: int = Field(..., description="ID of the MCP server") return_path: str = Field(..., description="Path to redirect to after callback") include_resource_param: bool = Field(..., description="Include resource parameter") + force_reauthentication: bool = Field( + default=False, + description="Ignore stored OAuth tokens and start a fresh authorization flow", + ) oauth_client_id: str | None = Field( None, description="OAuth client ID (optional for DCR)" ) diff --git a/backend/onyx/server/features/mcp/oauth.py b/backend/onyx/server/features/mcp/oauth.py index b6bb22bdfd2..8b08a02e945 100644 --- a/backend/onyx/server/features/mcp/oauth.py +++ b/backend/onyx/server/features/mcp/oauth.py @@ -227,6 +227,7 @@ async def connect_auto_discovery_oauth( connection_headers: dict[str, str], transport: MCPTransport, is_authenticated: bool, + force_reauthentication: bool = False, ) -> str | None: redirect_future = asyncio.get_running_loop().create_future() @@ -241,6 +242,18 @@ async def publish_redirect(auth_url: str) -> None: connection_config_id, admin_config_id, authorization_url_callback=publish_redirect, + load_stored_tokens=not force_reauthentication, + ) + + use_authenticated_connection = is_authenticated and not force_reauthentication + oauth_connection_headers = ( + { + key: value + for key, value in connection_headers.items() + if key.lower() != "authorization" + } + if force_reauthentication + else connection_headers ) async def initialize_or_start_oauth() -> None: @@ -248,12 +261,12 @@ async def initialize_or_start_oauth() -> None: # auth-capable Streamable HTTP probe for fresh SSE connections solely to # elicit a 401 challenge; authenticated connections use their real transport. probe_transport = ( - transport if is_authenticated else MCPTransport.STREAMABLE_HTTP + transport if use_authenticated_connection else MCPTransport.STREAMABLE_HTTP ) try: await initialize_mcp_client( mcp_server.server_url, - connection_headers=connection_headers, + connection_headers=oauth_connection_headers, transport=probe_transport, auth=oauth_auth, ) @@ -262,7 +275,7 @@ async def initialize_or_start_oauth() -> None: # or permits no Streamable HTTP initialization; well-known discovery # can still start OAuth. Once a token already existed or was refreshed, # however, this is a real authenticated initialization failure. - if is_authenticated or oauth_auth.context.is_token_valid(): + if use_authenticated_connection or oauth_auth.context.is_token_valid(): raise logger.info( "Initial MCP OAuth probe failed; trying well-known discovery", @@ -272,10 +285,10 @@ async def initialize_or_start_oauth() -> None: # Successful public initialization proves reachability, not consent. # Only a usable token makes the connection complete; otherwise force the # RFC 9728 well-known path so the user still receives a consent screen. - if is_authenticated or oauth_auth.context.is_token_valid(): + if use_authenticated_connection or oauth_auth.context.is_token_valid(): return await _initiate_oauth_from_well_known_metadata( - oauth_auth, mcp_server.server_url, connection_headers + oauth_auth, mcp_server.server_url, oauth_connection_headers ) try: @@ -290,7 +303,7 @@ async def initialize_or_start_oauth() -> None: if ( redirect_url is not None - or is_authenticated + or use_authenticated_connection or oauth_auth.context.is_token_valid() ): return redirect_url @@ -549,10 +562,13 @@ def __init__( connection_config_id: int, alt_config_id: int | None = None, refresh_log_context: MCPRefreshLogContext | None = None, + *, + load_stored_tokens: bool = True, ): self.alt_config_id = alt_config_id self.connection_config_id = connection_config_id self.refresh_log_context = refresh_log_context + self.load_stored_tokens = load_stored_tokens self.refresh_attempt_id: str | None = None # When bound, `get_tokens` hydrates its `token_expiry_time` from the # config read it already does — no separate query for the expiry. @@ -588,7 +604,7 @@ async def get_tokens(self) -> OAuthToken | None: OAuthMetadata.model_validate(metadata_raw) ) tokens_raw = config_data.get(MCPOAuthKeys.TOKENS.value) - if tokens_raw: + if tokens_raw and self.load_stored_tokens: return OAuthToken.model_validate(tokens_raw) return None @@ -832,6 +848,8 @@ def make_oauth_provider( connection_config_id: int, admin_config_id: int | None, authorization_url_callback: Callable[[str], Awaitable[None]] | None = None, + *, + load_stored_tokens: bool = True, ) -> OnyxOAuthClientProvider: async def redirect_handler(auth_url: str) -> None: if return_path == UNUSED_RETURN_PATH: @@ -891,6 +909,7 @@ async def callback_handler() -> tuple[str, str | None]: connection_config_id, admin_config_id, refresh_log_context, + load_stored_tokens=load_stored_tokens, ) provider = OnyxOAuthClientProvider( refresh_log_context=refresh_log_context, diff --git a/backend/tests/integration/tests/craft/docker_e2e/conftest.py b/backend/tests/integration/tests/craft/docker_e2e/conftest.py index 15ead5d2363..f5d2343e415 100644 --- a/backend/tests/integration/tests/craft/docker_e2e/conftest.py +++ b/backend/tests/integration/tests/craft/docker_e2e/conftest.py @@ -3,6 +3,7 @@ from __future__ import annotations import subprocess +import time from typing import NamedTuple, Protocol from uuid import UUID @@ -15,8 +16,20 @@ create_external_app, get_built_in_external_app, ) +from onyx.server.features.build.sandbox.docker.docker_sandbox_manager import ( + SANDBOX_EXEC_ENV, + SANDBOX_EXEC_USER, +) from tests.integration.common_utils.managers.build_session import BuildSessionManager from tests.integration.common_utils.test_models import DATestUser +from tests.integration.tests.craft.webapp_preview import ( + WEBAPP_BOOTSTRAP_TIMEOUT_S, + truncate_output, + verify_webapp_bootstrap, + webapp_bootstrap_command, + webapp_install_check_command, + webapp_logs_command, +) class DockerSandbox(NamedTuple): @@ -32,6 +45,7 @@ def __call__( *, timeout: float = 30.0, user: str | None = None, + env: dict[str, str] | None = None, ) -> subprocess.CompletedProcess[str]: ... @@ -69,10 +83,13 @@ def _docker_exec( *, timeout: float = 30.0, user: str | None = None, + env: dict[str, str] | None = None, ) -> subprocess.CompletedProcess[str]: command = ["docker", "exec"] if user is not None: command.extend(["--user", user]) + for key, value in (env or {}).items(): + command.extend(["-e", f"{key}={value}"]) command.extend([container, *cmd]) return subprocess.run( command, @@ -109,6 +126,56 @@ def _provision_sandbox( ) +def start_session_webapp(container: str, session_id: UUID) -> str: + """Run the lazy webapp bootstrap in-container, standing in for the agent's + `webapp` tool, and return the script's output. + + Carries the sandbox user's HOME as well as its uid: under the egress proxy + the container runs as root, so a bare ``--user 1000:1000`` would leave + HOME=/root and diverge from the agent's real privilege context. + """ + started_at = time.monotonic() + try: + result = _docker_exec( + container, + ["sh", "-c", webapp_bootstrap_command(session_id)], + timeout=WEBAPP_BOOTSTRAP_TIMEOUT_S, + user=SANDBOX_EXEC_USER, + env=SANDBOX_EXEC_ENV, + ) + except subprocess.TimeoutExpired: + pytest.fail( + f"start-webapp.sh did not finish within " + f"{WEBAPP_BOOTSTRAP_TIMEOUT_S}s for session {session_id}:\n" + f"{session_webapp_logs(container, session_id)}" + ) + elapsed_s = time.monotonic() - started_at + output = f"{result.stdout}{result.stderr}" + install_state = _docker_exec( + container, + ["sh", "-c", webapp_install_check_command(session_id)], + user=SANDBOX_EXEC_USER, + env=SANDBOX_EXEC_ENV, + ).stdout.strip() + verify_webapp_bootstrap( + session_id, + output=output, + install_state=install_state, + elapsed_s=elapsed_s, + ) + return output + + +def session_webapp_logs(container: str, session_id: UUID) -> str: + result = _docker_exec( + container, + ["sh", "-c", webapp_logs_command(session_id)], + user=SANDBOX_EXEC_USER, + env=SANDBOX_EXEC_ENV, + ) + return truncate_output(f"{result.stdout}{result.stderr}") + + @pytest.fixture(scope="session") def docker_exec() -> DockerExec: return _docker_exec diff --git a/backend/tests/integration/tests/craft/docker_e2e/test_webapp_preview_docker.py b/backend/tests/integration/tests/craft/docker_e2e/test_webapp_preview_docker.py index 1f74bcea5c7..703bb0c0707 100644 --- a/backend/tests/integration/tests/craft/docker_e2e/test_webapp_preview_docker.py +++ b/backend/tests/integration/tests/craft/docker_e2e/test_webapp_preview_docker.py @@ -1,11 +1,11 @@ """Webapp preview against a real Next dev server on the docker backend. -Docker counterpart of ``k8s/test_webapp_preview.py``: pins that -``DockerSandboxManager`` launches the shared Next.js start script at session -setup and that the resulting dev server serves the basePath route through the -proxy. A docker-only divergence from the shared start script (or a broken -basePath contract) fails here and nowhere else — the k8s suite only covers -the kubernetes manager's invocation. +Docker counterpart of ``k8s/test_webapp_preview.py``: drives the shared +``start-webapp.sh`` the way the agent's `webapp` tool does, then pins that the +resulting dev server serves the basePath route through the proxy. A +docker-only divergence from the shared start script (or a broken basePath +contract) fails here and nowhere else — the k8s suite only covers the +kubernetes manager's side. """ from __future__ import annotations @@ -20,6 +20,8 @@ from tests.integration.tests.craft.docker_e2e.conftest import ( ProvisionSandbox, remove_container, + session_webapp_logs, + start_session_webapp, ) from tests.integration.tests.craft.webapp_preview import ( proxy_get, @@ -31,8 +33,6 @@ reason="Docker integration tests require SANDBOX_BACKEND=docker.", ) -_WEBAPP_READY_TIMEOUT_S = 180.0 - @pytest.fixture def webapp_user() -> DATestUser: @@ -47,8 +47,16 @@ def test_preview_serves_at_base_path_and_reports_ready( sandbox = provision_sandbox(webapp_user, headless=False) try: session_id = str(sandbox.session_id) + bootstrap_output = start_session_webapp( + sandbox.container_name, sandbox.session_id + ) wait_for_webapp_ready( - webapp_user, session_id, timeout_s=_WEBAPP_READY_TIMEOUT_S + webapp_user, + session_id, + diagnostics=lambda: ( + f"--- start-webapp.sh ---\n{bootstrap_output}\n" + + session_webapp_logs(sandbox.container_name, sandbox.session_id) + ), ) resp = proxy_get(webapp_user, session_id) diff --git a/backend/tests/integration/tests/craft/docker_e2e/test_workspace_setup_docker.py b/backend/tests/integration/tests/craft/docker_e2e/test_workspace_setup_docker.py index 45a3a3175e0..0b01cd06a3a 100644 --- a/backend/tests/integration/tests/craft/docker_e2e/test_workspace_setup_docker.py +++ b/backend/tests/integration/tests/craft/docker_e2e/test_workspace_setup_docker.py @@ -24,7 +24,9 @@ DockerExec, DockerSandbox, ProvisionSandbox, + remove_container, ) +from tests.integration.tests.craft.webapp_preview import webapp_script_stat_command pytestmark = pytest.mark.skipif( SANDBOX_BACKEND != SandboxBackend.DOCKER, @@ -66,7 +68,6 @@ def test_session_setup_creates_user_writable_workspace( "/workspace/managed/user_library", session_path, f"{session_path}/outputs", - f"{session_path}/outputs/web", f"{session_path}/attachments", f"{session_path}/.opencode", ] @@ -111,11 +112,11 @@ def test_session_setup_creates_user_writable_workspace( 'printf ok > "/workspace/managed/.write-check"\n' f'printf ok > "{session_path}/.write-check"\n' f'printf ok > "{session_path}/attachments/.write-check"\n' - f'printf ok > "{session_path}/outputs/web/.write-check"\n' + f'printf ok > "{session_path}/outputs/.write-check"\n' 'rm -f "/workspace/managed/.write-check"\n' f'rm -f "{session_path}/.write-check"\n' f'rm -f "{session_path}/attachments/.write-check"\n' - f'rm -f "{session_path}/outputs/web/.write-check"\n' + f'rm -f "{session_path}/outputs/.write-check"\n' ), ], user=SANDBOX_EXEC_USER, @@ -177,3 +178,48 @@ def test_session_setup_creates_user_writable_workspace( ) post_delete_names = {entry.name for entry in post_delete_listing.entries} assert upload_name not in post_delete_names + + +def test_session_setup_writes_read_only_webapp_script( + workspace_user: DATestUser, + provision_sandbox: ProvisionSandbox, + docker_exec: DockerExec, +) -> None: + """A session with a preview port gets start-webapp.sh and nothing else. + + Webapp provisioning is lazy, so this script is all that setup leaves + behind for the agent to run. Mode 444 makes a stray redirect into the + script fail loudly; it is not a boundary against the agent, which owns + both the file and its directory. + """ + sandbox = provision_sandbox(workspace_user, headless=False) + try: + stat_result = docker_exec( + sandbox.container_name, + ["sh", "-c", webapp_script_stat_command(sandbox.session_id)], + user=SANDBOX_EXEC_USER, + ) + assert stat_result.returncode == 0, ( + "Expected start-webapp.sh after provisioning a session with a port. " + f"stdout={stat_result.stdout!r} stderr={stat_result.stderr!r}" + ) + assert stat_result.stdout.strip() == f"{SANDBOX_EXEC_USER} 444", ( + f"start-webapp.sh must be sandbox-user-owned and read-only: " + f"{stat_result.stdout!r}" + ) + + scaffolded = docker_exec( + sandbox.container_name, + [ + "sh", + "-c", + f'test -e "/workspace/sessions/{sandbox.session_id}/outputs/web" ' + "&& echo PRESENT || echo ABSENT", + ], + user=SANDBOX_EXEC_USER, + ).stdout.strip() + assert scaffolded == "ABSENT", ( + "Setup must not scaffold outputs/web; provisioning is lazy." + ) + finally: + remove_container(sandbox.container_name) diff --git a/backend/tests/integration/tests/craft/k8s/k8s_fixtures.py b/backend/tests/integration/tests/craft/k8s/k8s_fixtures.py index 604fa351d00..f28d2e9050d 100644 --- a/backend/tests/integration/tests/craft/k8s/k8s_fixtures.py +++ b/backend/tests/integration/tests/craft/k8s/k8s_fixtures.py @@ -33,11 +33,19 @@ from onyx.server.features.build.sandbox.kubernetes.kubernetes_sandbox_manager import ( KubernetesSandboxManager, ) +from onyx.server.features.build.sandbox.session_workspace import SESSIONS_ROOT from onyx.utils.logger import setup_logger from shared_configs.configs import POSTGRES_DEFAULT_SCHEMA_STANDARD_VALUE from shared_configs.contextvars import CURRENT_TENANT_ID_CONTEXTVAR from tests.integration.common_utils.managers.build_session import BuildSessionManager from tests.integration.common_utils.managers.user import UserManager +from tests.integration.tests.craft.webapp_preview import ( + WEBAPP_BOOTSTRAP_TIMEOUT_S, + verify_webapp_bootstrap, + webapp_bootstrap_command, + webapp_install_check_command, + webapp_logs_command, +) logger = setup_logger() @@ -468,10 +476,15 @@ def _cleanup_pool_workspace( "-mindepth 1 -delete 2>/dev/null; true", container="sidecar", ) + # Dev servers are nohup'd: without the kill they survive the delete and + # accumulate on the module-scoped pool pod until something OOMs. pod_exec( k8s_client, pod_name, SANDBOX_NAMESPACE, + # Bracketed so the patterns don't match this cleanup shell's own + # cmdline, which would kill it before the find runs. + "pkill -f 'bun run de[v]'; pkill -f 'next-serve[r]'; " "find /workspace/sessions -mindepth 1 -delete 2>/dev/null; true", container="sandbox", ) @@ -611,15 +624,18 @@ def pod_exec( namespace: str, command: str, container: str = "sandbox", + timeout_s: float | None = None, ) -> str: """Run a one-shot ``/bin/sh -c`` command in a pod container; return combined output. Pass ``container="sidecar"`` to write to ``/workspace/managed/`` (RO in the - sandbox container). + sandbox container). A lapsed ``timeout_s`` returns buffered output rather + than raising, so callers must verify the command's effect. """ from kubernetes.stream import stream as k8s_stream argv = ["/bin/sh", "-c", command] + optional_kwargs = {} if timeout_s is None else {"_request_timeout": timeout_s} resp = k8s_stream( client.connect_get_namespaced_pod_exec, name=pod_name, @@ -630,6 +646,7 @@ def pod_exec( stdin=False, stdout=True, tty=False, + **optional_kwargs, ) return str(resp) if resp is not None else "" @@ -831,9 +848,14 @@ def pool_session( Same shape as ``live_pod`` but reuses the pool pod. Use this unless the test mutates pod-level state (lifecycle/terminate/restart); those must use ``live_pod``. Sessions are headless (no dev server) by default; parametrize indirectly - with ``{"headless": False}`` for webapp/preview tests. + with ``{"headless": False}`` for webapp/preview tests, or use + ``webapp_pool_session`` to also get the webapp bootstrapped. """ headless = getattr(request, "param", {}).get("headless", True) + return _create_pool_session(_pool_pod, headless=headless) + + +def _create_pool_session(_pool_pod: _PoolPod, *, headless: bool) -> PoolSession: _cleanup_pool_workspace(_pool_pod.k8s_client, _pool_pod.pod_name) session_id, sandbox_id = BuildSessionManager.create_with_sandbox( _pool_pod.api_user, headless=headless @@ -855,6 +877,90 @@ def pool_session( ) +def start_session_webapp( + k8s_client: "k8s_client_module.CoreV1Api", + pod_name: str, + session_id: UUID, +) -> str: + """Run the lazy webapp bootstrap in-pod, standing in for the agent's + `webapp` tool, and return the script's output. + """ + started_at = time.monotonic() + output = pod_exec( + k8s_client, + pod_name, + SANDBOX_NAMESPACE, + webapp_bootstrap_command(session_id), + timeout_s=WEBAPP_BOOTSTRAP_TIMEOUT_S, + ) + elapsed_s = time.monotonic() - started_at + install_state = pod_exec( + k8s_client, + pod_name, + SANDBOX_NAMESPACE, + webapp_install_check_command(session_id), + ).strip() + verify_webapp_bootstrap( + session_id, + output=output, + install_state=install_state, + elapsed_s=elapsed_s, + ) + return output + + +def session_webapp_logs( + k8s_client: "k8s_client_module.CoreV1Api", + pod_name: str, + session_id: UUID, +) -> str: + return pod_exec( + k8s_client, + pod_name, + SANDBOX_NAMESPACE, + webapp_logs_command(session_id), + ) + + +def stop_session_webapp( + k8s_client: "k8s_client_module.CoreV1Api", + pod_name: str, + session_id: UUID, +) -> None: + """Stop a session's dev server and wait for it to actually exit. + + A live Turbopack keeps writing into the session tree, so a workspace + delete races it and leaves the directory behind — which then makes a + restore skip its reinstall, since the stale ``package.json`` is still + there. + """ + session_path = f"{SESSIONS_ROOT}/{session_id}" + pod_exec( + k8s_client, + pod_name, + SANDBOX_NAMESPACE, + f"if [ -f {session_path}/nextjs.pid ]; then " + f" pid=$(cat {session_path}/nextjs.pid); " + f" kill $pid 2>/dev/null; " + f" for _ in $(seq 1 20); do kill -0 $pid 2>/dev/null || break; sleep 1; done; " + f" kill -9 $pid 2>/dev/null; " + f"fi; true", + ) + + +@pytest.fixture(scope="function") +def webapp_pool_session(_pool_pod: _PoolPod) -> PoolSession: + """``pool_session`` with an installed ``outputs/web`` and no dev server. + + Its consumers measure install state; leaving the server up would have it + writing into the tree while they snapshot, delete, and restore it. + """ + session = _create_pool_session(_pool_pod, headless=False) + start_session_webapp(_pool_pod.k8s_client, session.pod_name, session.session_id) + stop_session_webapp(_pool_pod.k8s_client, session.pod_name, session.session_id) + return session + + @pytest.fixture(scope="function") def live_pod( k8s_manager: KubernetesSandboxManager, diff --git a/backend/tests/integration/tests/craft/k8s/test_bun_node_modules_dedup.py b/backend/tests/integration/tests/craft/k8s/test_bun_node_modules_dedup.py index 79a22c3e3de..7778b77c1db 100644 --- a/backend/tests/integration/tests/craft/k8s/test_bun_node_modules_dedup.py +++ b/backend/tests/integration/tests/craft/k8s/test_bun_node_modules_dedup.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import time from collections.abc import Generator from contextlib import suppress from typing import NamedTuple @@ -28,6 +29,7 @@ PoolSession, cleanup_api_user_sandbox_rows, pod_exec, + start_session_webapp, wait_for_pod_deletion, ) @@ -52,33 +54,64 @@ class TwoSessionPod(NamedTuple): # Snapshot without node_modules is a few MB; with it, ~150 MB. _MAX_SNAPSHOT_BYTES = 5 * 1024 * 1024 +# Outside the API's [3010, 3100) reservation range, so the hand-made second +# session can't collide with the API-created one on the same pod. +_SECOND_SESSION_NEXTJS_PORT = 3999 + def _setup_session( manager: KubernetesSandboxManager, sandbox_id: UUID, session_id: UUID, ) -> None: + """A port is required: without one setup writes no ``start-webapp.sh``, + and nothing else installs the ``node_modules`` dedup measures.""" manager.setup_session_workspace( sandbox_id=sandbox_id, session_id=session_id, llm_config=default_llm_config( api_key=os.environ.get("OPENAI_API_KEY", "test-key"), ), - nextjs_port=None, + nextjs_port=_SECOND_SESSION_NEXTJS_PORT, connectable_apps_section="", ) def _du_bytes(k8s_client: client.CoreV1Api, pod_name: str, path: str) -> int: + # .next is dev-server build output, not install state; counting it would + # fail this budget for a reason unrelated to hardlink dedup. raw = pod_exec( k8s_client, pod_name, SANDBOX_NAMESPACE, - f"du -sb {path} | awk '{{print $1}}'", + f"du -sb --exclude=.next {path} | awk '{{print $1}}'", ) return int(raw.strip()) +def _wait_for_file( + k8s_client: client.CoreV1Api, + pod_name: str, + path: str, + *, + timeout_s: float = 180.0, +) -> None: + """Restore returns as soon as its sentinel prints and the reinstall it + kicks off is nohup'd, so a single stat would race a bun install.""" + deadline = time.monotonic() + timeout_s + while time.monotonic() < deadline: + found = pod_exec( + k8s_client, + pod_name, + SANDBOX_NAMESPACE, + f"test -f {path} && echo YES || echo NO", + ).strip() + if found == "YES": + return + time.sleep(5.0) + pytest.fail(f"{path} never appeared within {timeout_s:.0f}s of restore") + + def _inode(k8s_client: client.CoreV1Api, pod_name: str, path: str) -> str: raw = pod_exec( k8s_client, @@ -96,11 +129,15 @@ def two_session_pod( ) -> Generator[TwoSessionPod, None, None]: """Sandbox + two bun-installed sessions: ``(sandbox_id, session_a, session_b, pod_name)``.""" api_user = UserManager.create(name=f"craft-k8s-{uuid4().hex[:8]}") - session_a, sandbox_id = BuildSessionManager.create_with_sandbox(api_user) + session_a, sandbox_id = BuildSessionManager.create_with_sandbox( + api_user, headless=False + ) pod_name = k8s_manager._get_pod_name(sandbox_id) session_b = uuid4() try: + start_session_webapp(k8s_client, pod_name, session_a) _setup_session(k8s_manager, sandbox_id, session_b) + start_session_webapp(k8s_client, pod_name, session_b) yield TwoSessionPod( sandbox_id=sandbox_id, session_a=session_a, @@ -117,11 +154,11 @@ def two_session_pod( def test_bun_cache_lives_on_workspace_volume( k8s_client: client.CoreV1Api, - pool_session: PoolSession, + webapp_pool_session: PoolSession, ) -> None: # Cross-mount hardlinks fall back to copies, so the cache must share a mount # with the session dirs. - _, _, pod_name = pool_session + _, _, pod_name = webapp_pool_session cache_mount = pod_exec( k8s_client, pod_name, @@ -178,10 +215,10 @@ def test_two_sessions_stay_under_dedup_budget( def test_snapshot_excludes_node_modules( - pool_session: PoolSession, + webapp_pool_session: PoolSession, pool_api_user: DATestUser, ) -> None: - _sandbox_id, session_id, _ = pool_session + _sandbox_id, session_id, _ = webapp_pool_session snapshot = BuildSessionManager.create_snapshot(pool_api_user, session_id) assert snapshot is not None, "snapshot should succeed for populated session" assert snapshot.size_bytes < _MAX_SNAPSHOT_BYTES, ( @@ -194,10 +231,10 @@ def test_snapshot_excludes_node_modules( def test_restored_session_hardlinks_to_cache( k8s_manager: KubernetesSandboxManager, k8s_client: client.CoreV1Api, - pool_session: PoolSession, + webapp_pool_session: PoolSession, pool_api_user: DATestUser, ) -> None: - sandbox_id, session_id, pod_name = pool_session + sandbox_id, session_id, pod_name = webapp_pool_session snapshot = BuildSessionManager.create_snapshot(pool_api_user, session_id) assert snapshot is not None @@ -210,16 +247,15 @@ def test_restored_session_hardlinks_to_cache( ) assert "MISSING" in gone + # Also the only live coverage of the restore script's autostart branch, + # which fires only because this fixture's session really installed. BuildSessionManager.restore(pool_api_user, session_id) - have_next = pod_exec( + _wait_for_file( k8s_client, pod_name, - SANDBOX_NAMESPACE, - f"test -f /workspace/sessions/{session_id}/outputs/web/node_modules/next/package.json " - f"&& echo YES || echo NO", - ).strip() - assert have_next == "YES", "restored session must have node_modules rebuilt" + f"/workspace/sessions/{session_id}/outputs/web/node_modules/next/package.json", + ) # Link count >= 2 means the file is hardlinked to the bun cache. nlink = pod_exec( @@ -238,10 +274,10 @@ def test_restored_session_hardlinks_to_cache( def test_agent_added_package_survives_snapshot_restore( k8s_manager: KubernetesSandboxManager, k8s_client: client.CoreV1Api, - pool_session: PoolSession, + webapp_pool_session: PoolSession, pool_api_user: DATestUser, ) -> None: - sandbox_id, session_id, pod_name = pool_session + sandbox_id, session_id, pod_name = webapp_pool_session session_web = f"/workspace/sessions/{session_id}/outputs/web" pod_exec( @@ -262,14 +298,9 @@ def test_agent_added_package_survives_snapshot_restore( assert snapshot is not None k8s_manager.cleanup_session_workspace(sandbox_id, session_id) BuildSessionManager.restore(pool_api_user, session_id) - have_lodash = pod_exec( - k8s_client, - pod_name, - SANDBOX_NAMESPACE, - f"test -f {session_web}/node_modules/lodash/package.json " - f"&& echo YES || echo NO", - ).strip() - assert have_lodash == "YES", "lodash should be rebuilt after restore via bun.lock" + _wait_for_file( + k8s_client, pod_name, f"{session_web}/node_modules/lodash/package.json" + ) def test_agent_install_in_one_session_does_not_break_other( diff --git a/backend/tests/integration/tests/craft/k8s/test_kubernetes_sandbox_file_ops.py b/backend/tests/integration/tests/craft/k8s/test_kubernetes_sandbox_file_ops.py index b6d638ae3d7..e3abe9a6821 100644 --- a/backend/tests/integration/tests/craft/k8s/test_kubernetes_sandbox_file_ops.py +++ b/backend/tests/integration/tests/craft/k8s/test_kubernetes_sandbox_file_ops.py @@ -154,8 +154,6 @@ def test_create_snapshot_returns_none_when_session_has_no_outputs( ) -> None: _sandbox_id, session_id, pod_name = pool_session - # Wipe snapshot-eligible trees so the session is truly empty (setup - # scaffolds outputs/web/). session_root = f"/workspace/sessions/{session_id}" pod_exec( k8s_client, diff --git a/backend/tests/integration/tests/craft/k8s/test_webapp_preview.py b/backend/tests/integration/tests/craft/k8s/test_webapp_preview.py index 24826de4800..b08376cc77e 100644 --- a/backend/tests/integration/tests/craft/k8s/test_webapp_preview.py +++ b/backend/tests/integration/tests/craft/k8s/test_webapp_preview.py @@ -7,13 +7,24 @@ from __future__ import annotations import pytest +from kubernetes import client -from onyx.server.features.build.configs import SANDBOX_BACKEND, SandboxBackend +from onyx.server.features.build.configs import ( + SANDBOX_BACKEND, + SANDBOX_NAMESPACE, + SandboxBackend, +) from tests.integration.common_utils.test_models import DATestUser -from tests.integration.tests.craft.k8s.k8s_fixtures import PoolSession +from tests.integration.tests.craft.k8s.k8s_fixtures import ( + PoolSession, + pod_exec, + session_webapp_logs, + start_session_webapp, +) from tests.integration.tests.craft.webapp_preview import ( proxy_get, wait_for_webapp_ready, + webapp_script_stat_command, ) pytestmark = [ @@ -29,33 +40,73 @@ ), ] -_WEBAPP_READY_TIMEOUT_S = 120.0 +@pytest.fixture +def ready_webapp_session( + pool_session: PoolSession, + pool_api_user: DATestUser, + k8s_client: client.CoreV1Api, +) -> PoolSession: + """A pool session whose dev server is up and serving. -def test_preview_serves_at_base_path_and_reports_ready( + Provisioning writes start-webapp.sh but scaffolds nothing, so the fixture + runs it the way the agent's `webapp` tool would. + """ + bootstrap_output = start_session_webapp( + k8s_client, pool_session.pod_name, pool_session.session_id + ) + wait_for_webapp_ready( + pool_api_user, + str(pool_session.session_id), + diagnostics=lambda: ( + f"--- start-webapp.sh ---\n{bootstrap_output}\n" + + session_webapp_logs( + k8s_client, pool_session.pod_name, pool_session.session_id + ) + ), + ) + return pool_session + + +def test_setup_writes_read_only_webapp_script( pool_session: PoolSession, + k8s_client: client.CoreV1Api, +) -> None: + """Setup's only webapp artifact, written through the k8s exec path. + + The docker suite pins the same bytes; both matter because the two managers + write the script separately. Mode 444 is what keeps a stray redirect from + clobbering the one file opencode's deny rules treat as fixed. + """ + stat_line = pod_exec( + k8s_client, + pool_session.pod_name, + SANDBOX_NAMESPACE, + webapp_script_stat_command(pool_session.session_id), + ).strip() + assert stat_line == "1000:1000 444", stat_line + + +def test_preview_serves_at_base_path_and_reports_ready( + ready_webapp_session: PoolSession, pool_api_user: DATestUser, ) -> None: """``ready`` flips true and the route users actually load returns 200.""" - session_id = str(pool_session.session_id) - wait_for_webapp_ready(pool_api_user, session_id, timeout_s=_WEBAPP_READY_TIMEOUT_S) - - resp = proxy_get(pool_api_user, session_id) + resp = proxy_get(pool_api_user, str(ready_webapp_session.session_id)) assert resp.status_code == 200, resp.text[:500] def test_dev_resources_not_blocked_by_origin_gate( - pool_session: PoolSession, + ready_webapp_session: PoolSession, pool_api_user: DATestUser, ) -> None: """A browser-shaped dev-resource request must not hit Next's cross-origin 403 (``blockCrossSiteDEV`` rejects /_next/* when the Origin hostname is not allowlisted — the exact failure from the 2026-07-06 incident).""" - session_id = str(pool_session.session_id) - wait_for_webapp_ready(pool_api_user, session_id, timeout_s=_WEBAPP_READY_TIMEOUT_S) - resp = proxy_get( - pool_api_user, session_id, "_next/static/onyx-origin-gate-probe.js" + pool_api_user, + str(ready_webapp_session.session_id), + "_next/static/onyx-origin-gate-probe.js", ) # 404 for a nonexistent asset is fine; the gate rejects before routing, # so a 403 means Origin/sec-fetch-* leaked through the proxy or the diff --git a/backend/tests/integration/tests/craft/webapp_preview.py b/backend/tests/integration/tests/craft/webapp_preview.py index 2df44c1354f..f42b8ba6ffa 100644 --- a/backend/tests/integration/tests/craft/webapp_preview.py +++ b/backend/tests/integration/tests/craft/webapp_preview.py @@ -3,24 +3,126 @@ The proxy contracts these pin (Origin/Sec-Fetch header stripping, basePath serving) must hold identically on every sandbox backend, so the k8s and docker suites assert them through this single implementation. + +Provisioning is lazy — setup only writes ``start-webapp.sh`` — so tests stand +in for the agent and run it themselves via ``start_session_webapp``. """ from __future__ import annotations import time +from collections.abc import Callable +from uuid import UUID import httpx import pytest +from onyx.server.features.build.sandbox.nextjs_dev import WEBAPP_PACKAGE_JSON_PATH +from onyx.server.features.build.sandbox.session_workspace import SESSIONS_ROOT from tests.integration.common_utils.constants import API_SERVER_URL from tests.integration.common_utils.http_client import client from tests.integration.common_utils.test_models import DATestUser _POLL_INTERVAL_S = 2.0 +# Hard exec cap only, so a wedged bootstrap fails with diagnostics rather than +# hanging; the budget it must actually meet is WEBAPP_TOOL_START_BUDGET_S. +WEBAPP_BOOTSTRAP_TIMEOUT_S = 420.0 +WEBAPP_READY_TIMEOUT_S = 300.0 + +# What the agent's `webapp` tool allows before it kills the bootstrap +# (START_TIMEOUT_MS in image/opencode-plugins/webapp.ts). +WEBAPP_TOOL_START_BUDGET_S = 150.0 + +# Neither backend's exec surfaces the script's nonzero exit, so this line is +# the only signal that the dev server actually came up. +WEBAPP_STARTED_SENTINEL = "dev server running on port" + +_MAX_DIAGNOSTIC_CHARS = 4000 + + +def truncate_output(output: str) -> str: + if len(output) <= _MAX_DIAGNOSTIC_CHARS: + return output + return f"...(truncated)\n{output[-_MAX_DIAGNOSTIC_CHARS:]}" + + +def webapp_bootstrap_command(session_id: UUID) -> str: + return f"bash {SESSIONS_ROOT}/{session_id}/start-webapp.sh" + + +def webapp_script_stat_command(session_id: UUID) -> str: + return f'stat -c "%u:%g %a" {SESSIONS_ROOT}/{session_id}/start-webapp.sh' + + +def webapp_logs_command(session_id: UUID, *, lines: int = 40) -> str: + session_path = f"{SESSIONS_ROOT}/{session_id}" + return ( + f"for log in {session_path}/webapp-bootstrap.log {session_path}/nextjs.log; do " + f'echo "--- $log ---"; tail -n {lines} "$log" 2>&1 || true; ' + f"done" + ) + + +WEBAPP_INSTALLED = "INSTALLED" + + +def webapp_install_check_command(session_id: UUID) -> str: + """Echoes how far provisioning got. + + Keys on the install marker, not the scaffold: the template copy writes + ``package.json`` before the bun install runs, so a scaffold check would + read a failed install as success. + """ + session_path = f"{SESSIONS_ROOT}/{session_id}" + web_path = f"{session_path}/outputs/web" + return ( + f"if [ -f {web_path}/node_modules/next/package.json ]; " + f"then echo {WEBAPP_INSTALLED}; " + f"elif [ -f {session_path}/{WEBAPP_PACKAGE_JSON_PATH} ]; " + f"then echo SCAFFOLD_ONLY; else echo MISSING; fi" + ) + + +def verify_webapp_bootstrap( + session_id: UUID, + *, + output: str, + install_state: str, + elapsed_s: float, +) -> None: + """Fail unless the bootstrap installed, started, and did so in budget. + + Neither backend's exec raises on the script's own failure paths, so + success has to be judged from what it printed and what it left behind. + """ + tail = truncate_output(output) + if install_state != WEBAPP_INSTALLED: + pytest.fail( + f"start-webapp.sh did not install outputs/web for session " + f"{session_id} (state={install_state}):\n{tail}" + ) + if WEBAPP_STARTED_SENTINEL not in output: + pytest.fail( + f"start-webapp.sh installed but never started a dev server for " + f"session {session_id}:\n{tail}" + ) + if elapsed_s > WEBAPP_TOOL_START_BUDGET_S: + pytest.fail( + f"start-webapp.sh took {elapsed_s:.0f}s for session {session_id}, " + f"over the {WEBAPP_TOOL_START_BUDGET_S:.0f}s the `webapp` tool " + f"allows before it kills the bootstrap (START_TIMEOUT_MS in " + f"image/opencode-plugins/webapp.ts). The agent's path is broken " + f"even though the script eventually finished." + ) + def wait_for_webapp_ready( - user: DATestUser, session_id: str, *, timeout_s: float + user: DATestUser, + session_id: str, + *, + timeout_s: float = WEBAPP_READY_TIMEOUT_S, + diagnostics: Callable[[], str] | None = None, ) -> None: deadline = time.monotonic() + timeout_s info: dict[str, object] = {} @@ -35,7 +137,8 @@ def wait_for_webapp_ready( if info.get("has_webapp") and info.get("ready"): return time.sleep(_POLL_INTERVAL_S) - pytest.fail(f"webapp never became ready within timeout: {info}") + detail = f"\n{diagnostics()}" if diagnostics is not None else "" + pytest.fail(f"webapp never became ready within timeout: {info}{detail}") def proxy_get(user: DATestUser, session_id: str, path: str = "") -> httpx.Response: diff --git a/backend/tests/unit/external_apps/test_google_cloud_scopes.py b/backend/tests/unit/external_apps/test_google_cloud_scopes.py new file mode 100644 index 00000000000..45b1a4782eb --- /dev/null +++ b/backend/tests/unit/external_apps/test_google_cloud_scopes.py @@ -0,0 +1,91 @@ +"""The cloud/self-hosted scope split for the Google providers: the cloud scope +stays clear of Google's restricted tier, and the catalog it exposes never +outruns it.""" + +from __future__ import annotations + +import pytest + +from onyx.db.enums import ExternalAppType +from onyx.external_apps.providers import gmail, google_calendar, google_drive, registry +from onyx.external_apps.providers.gmail import GmailAction +from onyx.external_apps.providers.google_drive import GoogleDriveAction +from onyx.external_apps.providers.registry import PROVIDERS, get_endpoint_catalog + +# Requesting any of these subjects the OAuth client to an annual third-party +# security assessment: https://support.google.com/cloud/answer/13464325 +_RESTRICTED_SCOPES = { + "https://mail.google.com/", + "https://www.googleapis.com/auth/gmail.readonly", + "https://www.googleapis.com/auth/gmail.metadata", + "https://www.googleapis.com/auth/gmail.modify", + "https://www.googleapis.com/auth/gmail.insert", + "https://www.googleapis.com/auth/gmail.compose", + "https://www.googleapis.com/auth/gmail.settings.basic", + "https://www.googleapis.com/auth/gmail.settings.sharing", + "https://www.googleapis.com/auth/drive", + "https://www.googleapis.com/auth/drive.readonly", + "https://www.googleapis.com/auth/drive.activity", + "https://www.googleapis.com/auth/drive.activity.readonly", + "https://www.googleapis.com/auth/drive.meet.readonly", + "https://www.googleapis.com/auth/drive.metadata", + "https://www.googleapis.com/auth/drive.metadata.readonly", + "https://www.googleapis.com/auth/drive.scripts", + # Legacy Drive-wide alias, still restricted. + "https://www.googleapis.com/auth/docs", +} + +_GOOGLE_APP_TYPES = [ + ExternalAppType.GMAIL, + ExternalAppType.GOOGLE_DRIVE, + ExternalAppType.GOOGLE_CALENDAR, +] +_CLOUD_SCOPES = [ + gmail._CLOUD_SCOPE, + google_drive._CLOUD_SCOPE, + google_calendar._CLOUD_SCOPE, +] + + +@pytest.fixture +def cloud(monkeypatch: pytest.MonkeyPatch) -> None: + """Pretend this deployment is cloud, where Onyx owns the OAuth client. Only + the catalog reads this — ``spec.oauth.scope`` is resolved at import.""" + monkeypatch.setattr(registry, "MULTI_TENANT", True) + + +@pytest.mark.parametrize("cloud_scope", _CLOUD_SCOPES) +def test_cloud_scope_avoids_restricted_scopes(cloud_scope: str) -> None: + assert not (set(cloud_scope.split()) & _RESTRICTED_SCOPES) + + +@pytest.mark.parametrize("app_type", _GOOGLE_APP_TYPES) +def test_self_hosted_keeps_the_full_catalog(app_type: ExternalAppType) -> None: + assert not registry.uses_cloud_scope(app_type) + assert get_endpoint_catalog(app_type) == PROVIDERS[app_type].spec.endpoint_catalog + + +@pytest.mark.usefixtures("cloud") +def test_gmail_is_send_only_on_cloud() -> None: + assert [e.id for e in get_endpoint_catalog(ExternalAppType.GMAIL)] == [ + GmailAction.MESSAGES_SEND + ] + + +@pytest.mark.usefixtures("cloud") +def test_drive_drops_only_shared_drive_listing_on_cloud() -> None: + """`drive.file` + the Docs API cover the rest of the catalog; only + drives.list needs a Drive-wide scope.""" + withheld = { + e.id for e in PROVIDERS[ExternalAppType.GOOGLE_DRIVE].spec.endpoint_catalog + } - {e.id for e in get_endpoint_catalog(ExternalAppType.GOOGLE_DRIVE)} + assert withheld == {GoogleDriveAction.DRIVES_READ} + + +@pytest.mark.usefixtures("cloud") +def test_calendar_catalog_survives_on_cloud() -> None: + """No Calendar scope is restricted, so the narrowed scope costs no actions.""" + assert ( + get_endpoint_catalog(ExternalAppType.GOOGLE_CALENDAR) + == PROVIDERS[ExternalAppType.GOOGLE_CALENDAR].spec.endpoint_catalog + ) diff --git a/backend/tests/unit/external_apps/test_google_drive_provider.py b/backend/tests/unit/external_apps/test_google_drive_provider.py index 30b9383e3af..51c44c1fdc6 100644 --- a/backend/tests/unit/external_apps/test_google_drive_provider.py +++ b/backend/tests/unit/external_apps/test_google_drive_provider.py @@ -35,7 +35,8 @@ def test_registered_as_managed_drive_provider() -> None: def test_scope_and_patterns_cover_read_and_upload() -> None: spec = _provider().spec - # The single `auth/drive` scope also authorizes the Google Docs API. + # The single `auth/drive` scope also authorizes the Google Docs API. Cloud + # requests a narrower scope instead — see test_google_cloud_scopes.py. assert spec.oauth.scope == "https://www.googleapis.com/auth/drive" # The /upload host path is required for content uploads to be token-injected; # the Docs API lives on its own `docs.googleapis.com` host. diff --git a/backend/tests/unit/onyx/server/features/mcp/test_mcp_oauth_connect.py b/backend/tests/unit/onyx/server/features/mcp/test_mcp_oauth_connect.py index 955ede36f7e..32e974c5389 100644 --- a/backend/tests/unit/onyx/server/features/mcp/test_mcp_oauth_connect.py +++ b/backend/tests/unit/onyx/server/features/mcp/test_mcp_oauth_connect.py @@ -74,9 +74,12 @@ def is_token_valid(self) -> bool: class _OAuthProvider(httpx.Auth): def __init__( - self, authorization_url_callback: Callable[[str], Awaitable[None]] + self, + authorization_url_callback: Callable[[str], Awaitable[None]], + load_stored_tokens: bool, ) -> None: self.authorization_url_callback = authorization_url_callback + self.load_stored_tokens = load_stored_tokens self.context = _OAuthContext() self.challenge: str | None = None @@ -121,10 +124,12 @@ def make_oauth_provider( _connection_config_id: int, _admin_config_id: int | None, authorization_url_callback: Callable[[str], Awaitable[None]] | None = None, + *, + load_stored_tokens: bool = True, ) -> _OAuthProvider: if authorization_url_callback is None: raise TypeError("authorization_url_callback is required") - provider = _OAuthProvider(authorization_url_callback) + provider = _OAuthProvider(authorization_url_callback, load_stored_tokens) providers.append(provider) return provider @@ -139,6 +144,7 @@ def _connect( *, is_authenticated: bool = False, connection_headers: dict[str, str] | None = None, + force_reauthentication: bool = False, ) -> str | None: return asyncio.run( oauth.connect_auto_discovery_oauth( @@ -150,6 +156,7 @@ def _connect( connection_headers=connection_headers or {}, transport=server.transport, is_authenticated=is_authenticated, + force_reauthentication=force_reauthentication, ) ) @@ -183,6 +190,35 @@ async def initialize_with_challenge( discovery_factory.assert_not_called() +def test_forced_reauthentication_skips_authenticated_fast_path( + monkeypatch: pytest.MonkeyPatch, +) -> None: + server, providers, initialize = _setup_coordinator(monkeypatch) + request_urls = _install_discovery_responses(monkeypatch, [200]) + + assert ( + _connect( + server, + is_authenticated=True, + connection_headers={ + "Authorization": "Bearer current-token", + "X-Gateway-Tenant": "tenant-1", + }, + force_reauthentication=True, + ) + == "https://consent" + ) + assert providers[0].load_stored_tokens is False + assert initialize.await_args is not None + assert initialize.await_args.kwargs["transport"] is MCPTransport.STREAMABLE_HTTP + assert initialize.await_args.kwargs["connection_headers"] == { + "X-Gateway-Tenant": "tenant-1" + } + assert request_urls == [ + "https://mcp.example.com/.well-known/oauth-protected-resource/mcp" + ] + + @pytest.mark.parametrize( ("transport", "initialization_error"), [ @@ -316,13 +352,16 @@ def handle_delegated(request: httpx.Request) -> httpx.Response: asyncio.run(run()) -def _request(server_id: int) -> MCPUserOAuthConnectRequest: +def _request( + server_id: int, *, force_reauthentication: bool = False +) -> MCPUserOAuthConnectRequest: return MCPUserOAuthConnectRequest( server_id=server_id, return_path="/admin/actions/mcp", include_resource_param=True, oauth_client_id="client-id", oauth_client_secret=None, + force_reauthentication=force_reauthentication, ) @@ -353,20 +392,33 @@ def _setup_api_connection( @pytest.mark.parametrize( - ("expires_at", "expected_oauth_url", "expected_discovery_urls"), + ( + "expires_at", + "force_reauthentication", + "expected_oauth_url", + "expected_discovery_urls", + ), [ - (4_000_000_000.0, "/admin/actions/mcp", []), + (4_000_000_000.0, False, "/admin/actions/mcp", []), + ( + 4_000_000_000.0, + True, + "https://consent", + ["https://mcp.example.com/.well-known/oauth-protected-resource/mcp"], + ), ( 1.0, + False, "https://consent", ["https://mcp.example.com/.well-known/oauth-protected-resource/mcp"], ), ], - ids=["valid", "expired"], + ids=["valid", "forced", "expired"], ) def test_token_expiry_controls_fast_path_without_dropping_oauth_state( monkeypatch: pytest.MonkeyPatch, expires_at: float, + force_reauthentication: bool, expected_oauth_url: str, expected_discovery_urls: list[str], ) -> None: @@ -388,7 +440,10 @@ def test_token_expiry_controls_fast_path_without_dropping_oauth_state( response = asyncio.run( api._connect_oauth( - _request(server.id), MagicMock(), is_admin=True, user=cast(User, user) + _request(server.id, force_reauthentication=force_reauthentication), + MagicMock(), + is_admin=True, + user=cast(User, user), ) ) diff --git a/backend/tests/unit/onyx/server/test_mcp_known_provider_oauth_helpers.py b/backend/tests/unit/onyx/server/test_mcp_known_provider_oauth_helpers.py index c129a13ed70..5a7972936c8 100644 --- a/backend/tests/unit/onyx/server/test_mcp_known_provider_oauth_helpers.py +++ b/backend/tests/unit/onyx/server/test_mcp_known_provider_oauth_helpers.py @@ -314,6 +314,36 @@ def test_get_tokens_hydrates_expiry_and_invalidates_expired_token( assert provider.context.is_token_valid() is False +def test_get_tokens_can_ignore_stored_token_for_reauthentication( + monkeypatch: pytest.MonkeyPatch, +) -> None: + provider = make_oauth_provider( + _make_mcp_server_stub(provider_mode=MCPOAuthProviderMode.AUTO_DISCOVERY), + user_id="user-1", + return_path="/return", + connection_config_id=1, + admin_config_id=None, + load_stored_tokens=False, + ) + _patch_config_read( + monkeypatch, + { + MCPOAuthKeys.TOKENS.value: { + "access_token": "current-token", + "token_type": "Bearer", + }, + MCPOAuthKeys.METADATA.value: { + "issuer": "https://accounts.example.com", + "authorization_endpoint": "https://accounts.example.com/authorize", + "token_endpoint": "https://accounts.example.com/token", + }, + }, + ) + + assert asyncio.run(provider.context.storage.get_tokens()) is None + assert provider.context.oauth_metadata is not None + + def test_get_tokens_clears_stale_expiry_when_absent( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/deployment/helm/charts/onyx/Chart.yaml b/deployment/helm/charts/onyx/Chart.yaml index 8e1c67a03c3..1c001ddfcbe 100644 --- a/deployment/helm/charts/onyx/Chart.yaml +++ b/deployment/helm/charts/onyx/Chart.yaml @@ -5,7 +5,7 @@ home: https://www.onyx.app/ sources: - "https://github.com/onyx-dot-app/onyx" type: application -version: 0.8.12 +version: 0.8.13 appVersion: latest annotations: category: Productivity @@ -16,21 +16,12 @@ annotations: - name: background image: docker.io/onyxdotapp/onyx-backend:latest artifacthub.io/changes: | - - kind: changed - description: values-localdev.yaml now disables the sandbox image prepuller. - Local kind clusters side-load the sandbox image (kind load + IfNotPresent), - so there is no registry pull to front-run, and with the localdev defaults - (:edge, pullPolicy Always) the prepuller would eagerly pull the ~3.3 GB - image from Docker Hub onto the kind node. Production values are unchanged. - kind: fixed - description: Fixed "helm upgrade" failing with 'PriorityClass ... value: Forbidden; - may not be changed in an update' after the sandbox image prepuller landed in - 0.8.6. PriorityClass.value is immutable and helm upgrade patches, so the - prepuller no longer ships a PriorityClass at all; it was buying very little. - Upgrading from 0.8.6 deletes it, and the prepuller runs at the cluster default - priority — set sandboxImagePrepull.priorityClassName to an existing class to - keep it preemptible. Running pods are unaffected, because pod priority is - resolved at admission. The chart now renders no cluster-scoped objects. + description: Route /scim to the api server in ingress mode (ingress.enabled=true). + The ingress templates only routed /api to the api server, so SCIM requests fell + through to the webserver and returned an HTML 404. A new ingress-scim.yaml + template forwards the /scim prefix, without a rewrite, to the api service on + the webserver host. dependencies: # Helm uses the first condition path that exists: `postgresqlOperator.enabled` # is unset by default, so this falls through to `postgresql.enabled`. Set it diff --git a/deployment/helm/charts/onyx/templates/ingress-scim.yaml b/deployment/helm/charts/onyx/templates/ingress-scim.yaml new file mode 100644 index 00000000000..f1fb3e3bc2d --- /dev/null +++ b/deployment/helm/charts/onyx/templates/ingress-scim.yaml @@ -0,0 +1,34 @@ +{{- if .Values.ingress.enabled -}} +# SCIM lives at the domain root (/scim/v2, outside /api) because IdPs address +# it directly. Separate resource: ingress-api's rewrite-target annotation is +# resource-wide and would strip the prefix. Bound to the webserver host, not +# the api host, because IdPs are configured with the browser-facing app domain. +apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + name: {{ include "onyx.fullname" . }}-ingress-scim + annotations: + {{- if not .Values.ingress.className }} + kubernetes.io/ingress.class: nginx + {{- end }} + cert-manager.io/cluster-issuer: {{ include "onyx.fullname" . }}-letsencrypt +spec: + {{- if .Values.ingress.className }} + ingressClassName: {{ .Values.ingress.className }} + {{- end }} + rules: + - host: {{ .Values.ingress.webserver.host }} + http: + paths: + - path: /scim + pathType: Prefix + backend: + service: + name: {{ include "onyx.fullname" . }}-api-service + port: + number: {{ .Values.api.service.servicePort }} + tls: + - hosts: + - {{ .Values.ingress.webserver.host }} + secretName: {{ include "onyx.fullname" . }}-ingress-scim-tls +{{- end }} diff --git a/web/src/refresh-components/buttons/LineItem.tsx b/web/src/refresh-components/buttons/LineItem.tsx index 6d99d9dccba..1180f98763a 100644 --- a/web/src/refresh-components/buttons/LineItem.tsx +++ b/web/src/refresh-components/buttons/LineItem.tsx @@ -235,6 +235,7 @@ export default function LineItem({ className={cn( "flex flex-row w-full items-start p-2 rounded-08 group/LineItem gap-2", children && description ? "items-start" : "items-center", + interactive && (disabled ? "cursor-not-allowed" : "cursor-pointer"), buttonClassNames[variant][emphasisKey] )} data-selected={selected} diff --git a/web/src/refresh-components/popovers/ActionsPopover/MCPLineItem.test.tsx b/web/src/refresh-components/popovers/ActionsPopover/MCPLineItem.test.tsx new file mode 100644 index 00000000000..50df18a4338 --- /dev/null +++ b/web/src/refresh-components/popovers/ActionsPopover/MCPLineItem.test.tsx @@ -0,0 +1,120 @@ +import { render, screen, setupUser } from "@tests/setup/test-utils"; +import { + MCPAuthenticationPerformer, + MCPAuthenticationType, + ToolSnapshot, +} from "@/lib/tools/interfaces"; +import MCPLineItem, { + MCPServer, +} from "@/refresh-components/popovers/ActionsPopover/MCPLineItem"; + +const oauthServer: MCPServer = { + id: 1, + name: "Test MCP server", + owner_email: "owner@example.com", + server_url: "https://mcp.example.com", + auth_type: MCPAuthenticationType.OAUTH, + auth_performer: MCPAuthenticationPerformer.PER_USER, + is_authenticated: false, +}; + +const tool: ToolSnapshot = { + id: 1, + name: "test_tool", + display_name: "Test tool", + description: "A test tool", + definition: null, + custom_headers: [], + in_code_tool_id: null, + passthrough_auth: false, + enabled: true, + chat_selectable: true, + agent_creation_selectable: true, + default_enabled: true, +}; + +interface RenderMCPLineItemOptions { + isAuthenticated?: boolean; + tools?: ToolSnapshot[]; +} + +function renderMCPLineItem({ + isAuthenticated = false, + tools = [], +}: RenderMCPLineItemOptions = {}) { + const onAuthenticate = jest.fn(); + const onSelect = jest.fn(); + + render( + + ); + + return { onAuthenticate, onSelect }; +} + +function getTrailingIndicator(row: HTMLElement): HTMLElement { + const indicators = row.querySelectorAll("[aria-hidden='true']"); + const indicator = indicators.item(indicators.length - 1); + if (!indicator) throw new Error("Expected a trailing MCP row indicator."); + return indicator; +} + +describe("MCPLineItem", () => { + it("authenticates once from either the row or key area", async () => { + const user = setupUser(); + const { onAuthenticate, onSelect } = renderMCPLineItem(); + const row = screen.getByRole("button", { name: oauthServer.name }); + + expect(screen.getAllByRole("button")).toHaveLength(1); + await user.click(row); + + expect(onAuthenticate).toHaveBeenCalledTimes(1); + expect(onSelect).not.toHaveBeenCalled(); + + onAuthenticate.mockClear(); + await user.click(getTrailingIndicator(row)); + + expect(onAuthenticate).toHaveBeenCalledTimes(1); + expect(onSelect).not.toHaveBeenCalled(); + }); + + it("authenticates once per keyboard activation", async () => { + const user = setupUser(); + const { onAuthenticate, onSelect } = renderMCPLineItem(); + const row = screen.getByRole("button", { name: oauthServer.name }); + + row.focus(); + await user.keyboard("{Enter}"); + + expect(onAuthenticate).toHaveBeenCalledTimes(1); + expect(onSelect).not.toHaveBeenCalled(); + + await user.keyboard(" "); + + expect(onAuthenticate).toHaveBeenCalledTimes(2); + expect(onSelect).not.toHaveBeenCalled(); + }); + + it("selects once when the chevron area is clicked", async () => { + const user = setupUser(); + const { onAuthenticate, onSelect } = renderMCPLineItem({ + isAuthenticated: true, + tools: [tool], + }); + const row = screen.getByRole("button", { name: oauthServer.name }); + + await user.click(getTrailingIndicator(row)); + + expect(onSelect).toHaveBeenCalledTimes(1); + expect(onAuthenticate).not.toHaveBeenCalled(); + }); +}); diff --git a/web/src/refresh-components/popovers/ActionsPopover/MCPLineItem.tsx b/web/src/refresh-components/popovers/ActionsPopover/MCPLineItem.tsx index 6cff5f8a3f6..3babc46081c 100644 --- a/web/src/refresh-components/popovers/ActionsPopover/MCPLineItem.tsx +++ b/web/src/refresh-components/popovers/ActionsPopover/MCPLineItem.tsx @@ -19,7 +19,6 @@ import { SvgSimpleLoader, } from "@opal/icons"; import { Section } from "@/layouts/general-layouts"; -import { Button } from "@opal/components"; import EnabledCount from "@/refresh-components/EnabledCount"; export interface MCPServer { @@ -114,20 +113,20 @@ export default function MCPLineItem({ /> )} {canClickIntoServer && ( -