diff --git a/src/api/catalog.ts b/src/api/catalog.ts index 235b1335..f97b9144 100644 --- a/src/api/catalog.ts +++ b/src/api/catalog.ts @@ -7,22 +7,6 @@ import type { GatewayTestResponse, } from "@/generated/types"; -export interface OAuthUserTokenStatus { - status: "valid" | "near_expiry" | "expired" | "missing"; - authorized: boolean; - scopes?: string[]; - expires_at?: string | null; - updated_at?: string | null; -} - -export interface OAuthGatewayStatus { - oauth_enabled: boolean; - grant_type?: string; - user_token_status?: OAuthUserTokenStatus; -} - -export type OAuthGatewayStatusMap = Record; - export interface GatewayImpactPreview { gatewayId: string; servers: Array<{ id: string; name: string }>; diff --git a/src/api/oauth.test.ts b/src/api/oauth.test.ts new file mode 100644 index 00000000..ec77af11 --- /dev/null +++ b/src/api/oauth.test.ts @@ -0,0 +1,120 @@ +import { describe, expect, it, vi } from "vitest"; +import { http, HttpResponse } from "msw"; + +import { server } from "@/test/mocks/server"; +import { getOAuthStatuses, normalizeGatewayIds } from "./oauth"; + +describe("normalizeGatewayIds", () => { + it("trims, removes empty values, deduplicates, and preserves order", () => { + expect(normalizeGatewayIds([" second ", "", "first", "second", " "])).toEqual([ + "second", + "first", + ]); + }); +}); + +describe("getOAuthStatuses", () => { + it("returns immediately for empty input", async () => { + const request = vi.fn(); + server.use( + http.get("*/api/oauth/status", () => { + request(); + return HttpResponse.json({}); + }), + ); + + await expect(getOAuthStatuses([])).resolves.toEqual({ statuses: {}, failures: {} }); + expect(request).not.toHaveBeenCalled(); + }); + + it("deduplicates IDs and keeps 100 IDs in one request", async () => { + const seen: string[][] = []; + const ids = Array.from({ length: 100 }, (_, index) => `gateway-${index}`); + + server.use( + http.get("*/api/oauth/status", ({ request }) => { + seen.push(new URL(request.url).searchParams.getAll("gateway_ids")); + return HttpResponse.json({}); + }), + ); + + await getOAuthStatuses([...ids, "gateway-99", " "]); + + expect(seen).toHaveLength(1); + expect(seen[0]).toHaveLength(100); + expect(seen[0]).toContain("gateway-99"); + }); + + it("rejects invalid gateway IDs before making a request", async () => { + const request = vi.fn(); + server.use( + http.get("*/api/oauth/status", () => { + request(); + return HttpResponse.json({}); + }), + ); + + await expect(getOAuthStatuses(["gateway/../secret"])).rejects.toThrow( + "Invalid server ID format", + ); + expect(request).not.toHaveBeenCalled(); + }); + + it("chunks 201 IDs and preserves successful batches when one batch fails", async () => { + const batchSizes: number[] = []; + server.use( + http.get("*/api/oauth/status", ({ request }) => { + const ids = new URL(request.url).searchParams.getAll("gateway_ids"); + batchSizes.push(ids.length); + if (ids[0] === "gateway-100") { + return HttpResponse.json({ detail: "failed" }, { status: 500 }); + } + return HttpResponse.json( + Object.fromEntries( + ids.map((id) => [ + id, + { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "valid", authorized: true }, + }, + ]), + ), + ); + }), + ); + + const result = await getOAuthStatuses( + Array.from({ length: 201 }, (_, index) => `gateway-${index}`), + ); + + expect(batchSizes).toEqual([100, 100, 1]); + expect(Object.keys(result.statuses)).toHaveLength(101); + expect(result.statuses["gateway-0"]?.user_token_status?.status).toBe("valid"); + expect(result.statuses["gateway-200"]?.user_token_status?.status).toBe("valid"); + expect(Object.keys(result.failures)).toHaveLength(100); + expect(result.failures["gateway-100"]).toEqual({ retryable: true, status: 500 }); + }); + + it("marks a forbidden batch unavailable and non-retryable", async () => { + server.use( + http.get("*/api/oauth/status", () => + HttpResponse.json({ detail: "forbidden" }, { status: 403 }), + ), + ); + + await expect(getOAuthStatuses(["gateway-1"])).resolves.toEqual({ + statuses: {}, + failures: { "gateway-1": { retryable: false, status: 403 } }, + }); + }); + + it("propagates abort instead of converting it to a batch failure", async () => { + const controller = new AbortController(); + controller.abort(); + + await expect(getOAuthStatuses(["gateway-1"], controller.signal)).rejects.toMatchObject({ + name: "AbortError", + }); + }); +}); diff --git a/src/api/oauth.ts b/src/api/oauth.ts new file mode 100644 index 00000000..04c25456 --- /dev/null +++ b/src/api/oauth.ts @@ -0,0 +1,91 @@ +import { api, ApiError } from "./client"; +import { validateServerId } from "@/utils/serverId"; + +export type OAuthTokenStatus = "valid" | "near_expiry" | "expired" | "missing" | "unknown"; + +export interface OAuthUserTokenStatus { + status: OAuthTokenStatus; + authorized: boolean; + scopes?: string[]; + expires_at?: string | null; + updated_at?: string | null; +} + +export interface OAuthGatewayStatus { + oauth_enabled: boolean; + grant_type?: string; + authorization_url?: string; + message?: string; + user_token_status?: OAuthUserTokenStatus; +} + +export type OAuthGatewayStatusMap = Record; + +export interface OAuthStatusFailure { + retryable: boolean; + status?: number; +} + +export interface OAuthStatusBatchResult { + statuses: OAuthGatewayStatusMap; + failures: Record; +} + +const OAUTH_STATUS_MAX_IDS = 100; + +export function normalizeGatewayIds(gatewayIds: string[]): string[] { + return [ + ...new Set( + gatewayIds + .map((id) => id.trim()) + .filter(Boolean) + .map(validateServerId), + ), + ]; +} + +/** Fetch caller-scoped OAuth state, respecting the backend's 100-id batch cap. */ +export async function getOAuthStatuses( + gatewayIds: string[], + signal?: AbortSignal, +): Promise { + const ids = normalizeGatewayIds(gatewayIds); + if (ids.length === 0) return { statuses: {}, failures: {} }; + + const batches: string[][] = []; + for (let start = 0; start < ids.length; start += OAUTH_STATUS_MAX_IDS) { + batches.push(ids.slice(start, start + OAUTH_STATUS_MAX_IDS)); + } + + const settled = await Promise.allSettled( + batches.map(async (batch) => { + const params = new URLSearchParams(); + batch.forEach((id) => params.append("gateway_ids", id)); + const statuses = await api.get( + `/oauth/status?${params.toString()}`, + undefined, + signal, + ); + return { batch, statuses }; + }), + ); + + if (signal?.aborted) throw new DOMException("Aborted", "AbortError"); + + const result: OAuthStatusBatchResult = { statuses: {}, failures: {} }; + settled.forEach((entry, index) => { + const batch = batches[index]; + if (entry.status === "fulfilled") { + Object.assign(result.statuses, entry.value.statuses); + return; + } + + const status = entry.reason instanceof ApiError ? entry.reason.status : undefined; + const failure = { retryable: status !== 403, ...(status === undefined ? {} : { status }) }; + batch.forEach((id) => { + result.failures[id] = failure; + }); + }); + + return result; +} diff --git a/src/api/servers.test.ts b/src/api/servers.test.ts index 9fb0b150..09f1cca9 100644 --- a/src/api/servers.test.ts +++ b/src/api/servers.test.ts @@ -499,61 +499,6 @@ describe("serversApi", () => { }); }); - describe("getOAuthStatus", () => { - const statusResponse = (body: unknown) => - new Response(JSON.stringify(body), { - status: 200, - headers: { "Content-Type": "application/json" }, - }); - - const idsFrom = (call: unknown[]) => - new URL(String(call[0]), "http://localhost").searchParams.getAll("gateway_ids"); - - it("sends one request and returns its statuses", async () => { - const body = { "srv-1": { user_token_status: { status: "missing" } } }; - mockFetch.mockResolvedValueOnce(statusResponse(body)); - - const result = await serversApi.getOAuthStatus(["srv-1"]); - - expect(result).toEqual(body); - expect(mockFetch).toHaveBeenCalledTimes(1); - expect(idsFrom(mockFetch.mock.calls[0])).toEqual(["srv-1"]); - }); - - it("splits over 100 ids across requests and merges the responses", async () => { - const ids = Array.from({ length: 101 }, (_, index) => `srv-${index}`); - mockFetch - .mockResolvedValueOnce( - statusResponse({ "srv-0": { user_token_status: { status: "valid" } } }), - ) - .mockResolvedValueOnce( - statusResponse({ "srv-100": { user_token_status: { status: "expired" } } }), - ); - - const result = await serversApi.getOAuthStatus(ids); - - expect(mockFetch).toHaveBeenCalledTimes(2); - expect(idsFrom(mockFetch.mock.calls[0])).toHaveLength(100); - expect(idsFrom(mockFetch.mock.calls[1])).toEqual(["srv-100"]); - expect(result).toEqual({ - "srv-0": { user_token_status: { status: "valid" } }, - "srv-100": { user_token_status: { status: "expired" } }, - }); - }); - - it("makes no request for an empty id list", async () => { - await expect(serversApi.getOAuthStatus([])).resolves.toEqual({}); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it("rejects a malformed id before any request", async () => { - await expect(serversApi.getOAuthStatus(["../etc"])).rejects.toThrow( - "Invalid server ID format", - ); - expect(mockFetch).not.toHaveBeenCalled(); - }); - }); - describe("testConnection", () => { it("POSTs /v1/mcp-servers/:id/test and returns the result", async () => { mockFetch.mockResolvedValueOnce( diff --git a/src/api/servers.ts b/src/api/servers.ts index 7e1271d5..44ec17ea 100644 --- a/src/api/servers.ts +++ b/src/api/servers.ts @@ -6,7 +6,7 @@ */ import { api } from "./client"; -import type { ServersResponse, MCPServer, GatewayOAuthStatus } from "../types/server"; +import type { ServersResponse, MCPServer } from "../types/server"; import type { GatewayHandshakeRequest, GatewayHandshakeResponse, @@ -14,12 +14,10 @@ import type { GatewayTestRequest, GatewayTestResponse, } from "@/generated/types"; +import { validateServerId } from "@/utils/serverId"; const serverByIdRequestCache = new Map>(); -/** Mirrors OAUTH_STATUS_BATCH_MAX_IDS on the backend's /oauth/status route. */ -const OAUTH_STATUS_MAX_IDS = 100; - /** The user closed the OAuth popup. Typed so callers can stay quiet about it. */ export class OAuthCancelledError extends Error { constructor() { @@ -28,25 +26,6 @@ export class OAuthCancelledError extends Error { } } -/** - * Validates server ID to prevent path traversal and injection attacks - * @param id - The server ID to validate - * @returns The validated ID - * @throws Error if ID is invalid - */ -function validateServerId(id: string): string { - if (!id || typeof id !== "string") { - throw new Error("Invalid server ID"); - } - - // Ensure ID is alphanumeric with hyphens/underscores only - if (!/^[a-zA-Z0-9_-]+$/.test(id)) { - throw new Error("Invalid server ID format"); - } - - return id; -} - function openOAuthAuthorizationPopup(): Window { const width = 600; const height = 700; @@ -214,31 +193,6 @@ export const serversApi = { return api.post(`/oauth/fetch-tools/${validId}`); }, - /** - * The caller's own OAuth state for each gateway, batched. - * - * Keys stay snake_case, unlike the gateway endpoints. Ids that are missing or - * not visible to the caller are omitted. A paged-through list can exceed the - * backend's id cap, so requests are split and the responses merged. - */ - getOAuthStatus: async (ids: string[]): Promise> => { - const validIds = ids.map(validateServerId); - const batches: string[][] = []; - for (let start = 0; start < validIds.length; start += OAUTH_STATUS_MAX_IDS) { - batches.push(validIds.slice(start, start + OAUTH_STATUS_MAX_IDS)); - } - - const responses = await Promise.all( - batches.map((batch) => { - const params = new URLSearchParams(); - batch.forEach((id) => params.append("gateway_ids", id)); - return api.get>(`/oauth/status?${params.toString()}`); - }), - ); - - return Object.assign({}, ...responses) as Record; - }, - /** * Open blank OAuth popup during an active user gesture. * diff --git a/src/components/gateways/SourceSelection.test.tsx b/src/components/gateways/SourceSelection.test.tsx index 7bda16bd..f862f84f 100644 --- a/src/components/gateways/SourceSelection.test.tsx +++ b/src/components/gateways/SourceSelection.test.tsx @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { Bot, Code2, Grid3x3, Wrench } from "lucide-react"; @@ -8,6 +8,15 @@ import type { ActionCard } from "@/components/gateways/types"; import { server } from "@/test/mocks/server"; import { renderWithProviders } from "@/test/test-utils"; +const authState = vi.hoisted(() => ({ + hasPermission: vi.fn<(permission: string) => boolean>(() => true), + permissionsLoading: false, +})); + +vi.mock("@/auth/useAuth", () => ({ + useAuth: () => authState, +})); + const actionCards: ActionCard[] = [ { icon: Wrench, @@ -61,6 +70,11 @@ function buildFourActionCards() { } describe("SourceSelection", () => { + beforeEach(() => { + authState.hasPermission.mockReturnValue(true); + authState.permissionsLoading = false; + }); + it("lazy-loads selectable MCP servers and hides sources already in the virtual server", async () => { const user = userEvent.setup(); let gatewaysRequestCount = 0; @@ -196,6 +210,119 @@ describe("SourceSelection", () => { expect(await screen.findByRole("alert")).toHaveTextContent("HTTP 500"); }); + it("does not load MCP servers without gateways.read", async () => { + const user = userEvent.setup(); + const gatewaysRequest = vi.fn(); + const oauthStatusRequest = vi.fn(); + authState.hasPermission.mockReturnValue(false); + server.use( + http.get("*/v1/mcp-servers", () => { + gatewaysRequest(); + return HttpResponse.json({ gateways: [] }); + }), + http.get("*/api/oauth/status", () => { + oauthStatusRequest(); + return HttpResponse.json({}); + }), + ); + + renderWithProviders( + , + ); + await user.click( + screen.getByRole("button", { + name: "Add tools, resources, and prompts from connected sources", + }), + ); + + expect(await screen.findByText("No MCP servers found.")).toBeInTheDocument(); + expect(gatewaysRequest).not.toHaveBeenCalled(); + expect(oauthStatusRequest).not.toHaveBeenCalled(); + }); + + it("keeps OAuth sources selectable and retries unavailable caller status", async () => { + const user = userEvent.setup(); + let statusRequests = 0; + server.use( + http.get("*/v1/mcp-servers", () => + HttpResponse.json({ + gateways: [ + { + id: "oauth-source", + name: "OAuth source", + url: "https://source.example/mcp", + transport: "SSE", + enabled: true, + reachable: true, + visibility: "public", + authType: "oauth", + tool_count: 1, + resource_count: 0, + prompt_count: 0, + }, + ], + }), + ), + http.get("*/api/oauth/status", () => { + statusRequests += 1; + if (statusRequests === 1) { + return HttpResponse.json({ detail: "temporary failure" }, { status: 500 }); + } + return HttpResponse.json({ + "oauth-source": { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "missing", authorized: false }, + }, + }); + }), + ); + + renderWithProviders( + , + ); + await user.click( + screen.getByRole("button", { + name: "Add tools, resources, and prompts from connected sources", + }), + ); + + const checkbox = await screen.findByRole("checkbox", { name: "Select OAuth source" }); + expect(checkbox).toBeEnabled(); + expect( + await screen.findByRole("button", { + name: "OAuth source status: Authorization status unavailable. Show details", + }), + ).toBeInTheDocument(); + + await user.click(checkbox); + expect(checkbox).toBeChecked(); + await user.click( + screen.getByRole("button", { + name: "OAuth source status: Authorization status unavailable. Show details", + }), + ); + await user.click(await screen.findByRole("button", { name: "Retry status" })); + + expect( + await screen.findByRole("button", { + name: "OAuth source status: Authorization required. Show details", + }), + ).toBeInTheDocument(); + expect( + await screen.findByText( + "Authorize from Server catalog or MCP Servers. Your wizard selections will be preserved.", + ), + ).toBeInTheDocument(); + expect(checkbox).toBeChecked(); + }); + it("shows the connect button only on the initially selected card", () => { const { cards } = buildFourActionCards(); diff --git a/src/components/gateways/SourceSelection.tsx b/src/components/gateways/SourceSelection.tsx index 9efa0f73..97d6df7e 100644 --- a/src/components/gateways/SourceSelection.tsx +++ b/src/components/gateways/SourceSelection.tsx @@ -1,13 +1,11 @@ import { useMemo, useState } from "react"; import { useIntl } from "react-intl"; import { - Activity, ArrowLeft, Box, Building2, ChevronDown, ChevronRight, - CircleSlash, Lock, MessageSquareCode, Plus, @@ -16,15 +14,18 @@ import { } from "lucide-react"; import { MainNavIcon } from "@/components/icons/MainNavIcon"; import { MCPIcon } from "@/components/icons/MCPIcon"; +import { useAuth } from "@/auth/useAuth"; import { Button } from "@/components/ui/button"; import { Checkbox } from "@/components/ui/checkbox"; import { Loading } from "@/components/ui/loading"; import { TruncatedText } from "@/components/ui/truncated-text"; import type { ActionCard } from "@/components/gateways/types"; +import { ServerStatusIndicator } from "@/components/servers/ServerStatusIndicator"; +import { useOAuthStatuses } from "@/hooks/useOAuthStatuses"; import { useQuery } from "@/hooks/useQuery"; +import { isOAuthServer } from "@/lib/serverStatus"; import { cn } from "@/lib/utils"; -import { STATUS_ICON } from "@/lib/status"; -import type { MCPServer, ServerStatus } from "@/types/server"; +import type { MCPServer } from "@/types/server"; const MCP_SERVERS_QUERY_PATH = "/v1/mcp-servers?limit=100&include_inactive=true"; @@ -60,41 +61,6 @@ function getPromptCount(server: ListedMCPServer) { return server.promptCount ?? server.prompt_count ?? 0; } -function getServerStatus(server: ListedMCPServer): ServerStatus { - if (!server.enabled) return "draft"; - if (!server.reachable) return server.lastSeen ? "warning" : "offline"; - return "active"; -} - -function getStatusConfig(status: ServerStatus) { - switch (status) { - case "active": - return { - Icon: Activity, - labelId: "gateways.source.status.active", - className: "text-success", - }; - case "warning": - return { - Icon: STATUS_ICON.warning, - labelId: "gateways.source.status.warning", - className: "text-warning", - }; - case "offline": - return { - Icon: CircleSlash, - labelId: "gateways.source.status.offline", - className: "text-muted-foreground", - }; - default: - return { - Icon: CircleSlash, - labelId: "gateways.source.status.inactive", - className: "text-muted-foreground", - }; - } -} - function getVisibilityConfig(visibility: ListedMCPServer["visibility"]) { switch (visibility) { case "private": @@ -123,6 +89,8 @@ export function SourceSelection({ }; }) { const intl = useIntl(); + const { hasPermission, permissionsLoading } = useAuth(); + const canReadMCPServers = !permissionsLoading && hasPermission("gateways.read"); const firstEnabledIndex = actionCards.findIndex((card) => !card.disabled); const initialSelectedIndex = firstEnabledIndex === -1 ? 0 : firstEnabledIndex; const [selectedIndex, setSelectedIndex] = useState(initialSelectedIndex); @@ -135,9 +103,16 @@ export function SourceSelection({ error: mcpServersError, isLoading: mcpServersLoading, } = useQuery(MCP_SERVERS_QUERY_PATH, { - enabled: Boolean(createServerActions) && hasRequestedMCPServers, + enabled: Boolean(createServerActions) && hasRequestedMCPServers && canReadMCPServers, }); const mcpServers = useMemo(() => getMCPServers(mcpServersData), [mcpServersData]); + const oauthServerIds = useMemo( + () => mcpServers.filter(isOAuthServer).map((server) => server.id), + [mcpServers], + ); + const { entries: oauthStatuses, retry: retryOAuthStatus } = useOAuthStatuses(oauthServerIds, { + enabled: canReadMCPServers, + }); const associatedMCPServerIdSet = useMemo( () => new Set(associatedMCPServerIds), [associatedMCPServerIds], @@ -382,8 +357,6 @@ export function SourceSelection({ const promptCount = getPromptCount(server); const visibility = getVisibilityConfig(server.visibility); const VisibilityIcon = visibility.Icon; - const status = getStatusConfig(getServerStatus(server)); - const StatusIcon = status.Icon; const isSelected = selectedMCPServerIds.has(server.id); return ( @@ -429,10 +402,14 @@ export function SourceSelection({ {intl.formatMessage({ id: visibility.labelId })} - - - {intl.formatMessage({ id: status.labelId })} - + void retryOAuthStatus(server.id)} + authorizationManagementHint + compact + className="justify-self-start" + /> ); })} diff --git a/src/components/server-catalog/CatalogResults.test.tsx b/src/components/server-catalog/CatalogResults.test.tsx index fccbb3f2..5505de43 100644 --- a/src/components/server-catalog/CatalogResults.test.tsx +++ b/src/components/server-catalog/CatalogResults.test.tsx @@ -140,8 +140,13 @@ describe("CatalogResults", () => { canDisconnect={false} oauthStatuses={{ "gateway-github": { - oauth_enabled: true, - user_token_status: { status: "expired", authorized: false }, + state: "ready", + tokenStatus: "expired", + status: { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "expired", authorized: false }, + }, }, }} />, @@ -184,8 +189,13 @@ describe("CatalogResults", () => { canDisconnect oauthStatuses={{ "gateway-github": { - oauth_enabled: true, - user_token_status: { status: "expired", authorized: false }, + state: "ready", + tokenStatus: "expired", + status: { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "expired", authorized: false }, + }, }, }} />, @@ -239,12 +249,22 @@ describe("CatalogResults", () => { canDisconnect={false} oauthStatuses={{ "gateway-valid": { - oauth_enabled: true, - user_token_status: { status: "valid", authorized: true }, + state: "ready", + tokenStatus: "valid", + status: { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "valid", authorized: true }, + }, }, "gateway-expiring": { - oauth_enabled: true, - user_token_status: { status: "near_expiry", authorized: true }, + state: "ready", + tokenStatus: "near_expiry", + status: { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "near_expiry", authorized: true }, + }, }, }} />, @@ -254,8 +274,7 @@ describe("CatalogResults", () => { expect(screen.getByText("Connected")).toBeInTheDocument(); }); - it("shows authorization needed when OAuth configuration is incomplete", async () => { - const user = userEvent.setup(); + it("does not infer authorization from the catalog configuration flag", () => { const onAuthorize = vi.fn(); const server = { ...availableServer, @@ -282,13 +301,12 @@ describe("CatalogResults", () => { />, ); - expect(screen.getByText("Needs authorization")).toBeInTheDocument(); - await user.click(screen.getByRole("button", { name: "Actions for GitHub pending" })); - await user.click(screen.getByRole("menuitem", { name: "Authorize" })); - expect(onAuthorize).toHaveBeenCalledOnce(); + expect(screen.getByText("Authorization status unavailable")).toBeInTheDocument(); + expect(screen.queryByText("Needs authorization")).not.toBeInTheDocument(); + expect(onAuthorize).not.toHaveBeenCalled(); }); - it("keeps a registered OAuth card connected while token status is unavailable", () => { + it("never shows a registered OAuth card as connected while token status is unavailable", () => { const server = { ...availableServer, id: "github-status-pending", @@ -313,7 +331,8 @@ describe("CatalogResults", () => { />, ); - expect(screen.getByText("Connected")).toBeInTheDocument(); + expect(screen.getByText("Authorization status unavailable")).toBeInTheDocument(); + expect(screen.queryByText("Connected")).not.toBeInTheDocument(); }); it("shows unregistered catalog servers as not connected in details", async () => { @@ -324,6 +343,24 @@ describe("CatalogResults", () => { expect(screen.getByText("Not connected")).toBeInTheDocument(); }); + it("never labels unavailable OAuth status connected in details", () => { + renderWithProviders( + , + ); + + expect(screen.getByText("Authorization status unavailable")).toBeInTheDocument(); + expect(screen.queryByText("Connected")).not.toBeInTheDocument(); + }); + it("routes bundled catalog logos through the BFF", () => { const { container } = renderWithProviders( catalogResults({ ...availableServer, logo_url: "/static/catalog-icons/asana.png" }), diff --git a/src/components/server-catalog/CatalogResults.tsx b/src/components/server-catalog/CatalogResults.tsx index 7d47fdb2..f3c93e47 100644 --- a/src/components/server-catalog/CatalogResults.tsx +++ b/src/components/server-catalog/CatalogResults.tsx @@ -3,7 +3,7 @@ import type { ReactNode } from "react"; import { EllipsisVertical, FileText, KeyRound, Lock, Plus } from "lucide-react"; import { useIntl } from "react-intl"; import { STATUS_ICON, STATUS_TONE_CLASS } from "@/lib/status"; -import type { OAuthGatewayStatus } from "@/api/catalog"; +import { isRetryableOAuthStatus, type OAuthStatusEntry } from "@/hooks/useOAuthStatuses"; import { EmptyStatePlaceholder } from "@/components/dashboard/EmptyStatePlaceholder"; import { CatalogLogo } from "@/components/server-catalog/CatalogLogo"; @@ -25,45 +25,92 @@ import { } from "@/components/ui/dropdown-menu"; import type { CatalogServer } from "@/generated/types"; import { useDebouncedValue } from "@/hooks/useDebouncedValue"; -import { getAuthTypeGroupId, getAuthTypeGroupLabelId } from "@/utils/catalogAuthTypes"; +import { + getAuthTypeGroupId, + getAuthTypeGroupLabelId, + isCatalogOAuthServer, +} from "@/utils/catalogAuthTypes"; import { getTagLabels } from "@/utils/tags"; const EMPTY_PENDING_IDS: ReadonlySet = new Set(); -const EMPTY_OAUTH_STATUSES: Readonly> = {}; +const EMPTY_OAUTH_STATUSES: Readonly> = {}; -function getOAuthCardState(server: CatalogServer, status?: OAuthGatewayStatus) { - const tokenState = status?.user_token_status?.status; +function getOAuthCardState(server: CatalogServer, entry?: OAuthStatusEntry) { + if (!isCatalogOAuthServer(server)) + return { + messageId: "mcpServer.catalog.connected", + severity: "success" as const, + canAuthorize: false, + canRetry: false, + }; + if (!entry || entry.state === "unavailable") + return { + messageId: "mcpServer.catalog.oauth.statusUnavailable", + severity: "warning" as const, + canAuthorize: false, + canRetry: isRetryableOAuthStatus(entry), + }; + if (entry.state === "loading") + return { + messageId: "mcpServer.catalog.oauth.statusChecking", + severity: "info" as const, + canAuthorize: false, + canRetry: false, + }; + if (entry.state === "not_applicable") + return { + messageId: "mcpServer.catalog.connected", + severity: "success" as const, + canAuthorize: false, + canRetry: false, + }; + + const tokenState = entry.tokenStatus; if (tokenState === "valid") return { messageId: "mcpServer.catalog.connected", severity: "success" as const, canAuthorize: false, + canRetry: false, }; if (tokenState === "near_expiry") return { messageId: "mcpServer.catalog.oauth.nearExpiry", severity: "warning" as const, canAuthorize: false, + canRetry: false, }; if (tokenState === "expired") return { messageId: "mcpServer.catalog.oauth.expired", severity: "error" as const, canAuthorize: true, + canRetry: false, }; - if (server.requires_oauth_config || tokenState === "missing") + if (tokenState === "missing") return { messageId: "mcpServer.catalog.oauth.needsAuthorization", severity: "info" as const, canAuthorize: true, + canRetry: false, }; return { - messageId: "mcpServer.catalog.connected", - severity: "success" as const, + messageId: "mcpServer.catalog.oauth.statusUnavailable", + severity: "warning" as const, canAuthorize: false, + canRetry: true, }; } +function isOAuthCardUsable(server: CatalogServer, entry?: OAuthStatusEntry): boolean { + if (!isCatalogOAuthServer(server)) return true; + if (entry?.state === "not_applicable") return true; + return ( + entry?.state === "ready" && + (entry.tokenStatus === "valid" || entry.tokenStatus === "near_expiry") + ); +} + function CatalogCard({ server, onView, @@ -77,6 +124,7 @@ function CatalogCard({ canTest, canDisconnect, oauthStatuses, + onRetryOAuthStatus, }: { server: CatalogServer; onView: (trigger: HTMLElement) => void; @@ -89,7 +137,8 @@ function CatalogCard({ isDisconnecting: boolean; canTest: boolean; canDisconnect: boolean; - oauthStatuses?: Readonly>; + oauthStatuses?: Readonly>; + onRetryOAuthStatus: (gatewayId: string) => void; }) { const intl = useIntl(); const headingId = useId(); @@ -101,6 +150,10 @@ function CatalogCard({ server, server.gateway_id ? oauthStatuses?.[server.gateway_id] : undefined, ); + const oauthUsable = isOAuthCardUsable( + server, + server.gateway_id ? oauthStatuses?.[server.gateway_id] : undefined, + ); const authTypeGroupId = getAuthTypeGroupId(server.auth_type); const requiresAuthentication = authTypeGroupId === "apiKey" || authTypeGroupId === "oauth"; const StatusIcon = STATUS_ICON[oauthState.severity]; @@ -204,12 +257,10 @@ function CatalogCard({ {canTest && ( )} + {oauthState.canRetry && server.gateway_id && ( + onRetryOAuthStatus(server.gateway_id!)} + > + {intl.formatMessage({ id: "mcpServer.catalog.oauth.retryStatus" })} + + )} {canDisconnect && server.gateway_id && ( void; }) { const intl = useIntl(); const tagsHeadingId = useId(); const tagLabels = getTagLabels(server?.tags ?? []); const authTypeLabelId = server ? getAuthTypeGroupLabelId(server.auth_type) : null; + const registeredStatus = server ? getOAuthCardState(server, oauthStatus) : null; return ( @@ -336,7 +398,7 @@ export function CatalogServerDetailsDialog({ )} {server.is_registered - ? intl.formatMessage({ id: "mcpServer.catalog.connected" }) + ? intl.formatMessage({ id: registeredStatus!.messageId }) : intl.formatMessage({ id: "mcpServer.catalog.notConnected" })} @@ -375,6 +437,7 @@ export function CatalogResults({ canTest, canDisconnect, oauthStatuses = EMPTY_OAUTH_STATUSES, + onRetryOAuthStatus = () => undefined, }: { servers: CatalogServer[]; emptyStateMessageId: string; @@ -388,7 +451,8 @@ export function CatalogResults({ disconnectingServerIds?: ReadonlySet; canTest: boolean; canDisconnect: boolean; - oauthStatuses?: Readonly>; + oauthStatuses?: Readonly>; + onRetryOAuthStatus?: (gatewayId: string) => void; }) { const intl = useIntl(); const announcedCount = useDebouncedValue(servers.length, 300); @@ -418,6 +482,7 @@ export function CatalogResults({ canTest={canTest} canDisconnect={canDisconnect} oauthStatuses={oauthStatuses} + onRetryOAuthStatus={onRetryOAuthStatus} /> ))} diff --git a/src/components/servers/MCPServerDetailsPanel.test.tsx b/src/components/servers/MCPServerDetailsPanel.test.tsx index 7d7310c5..1bc83440 100644 --- a/src/components/servers/MCPServerDetailsPanel.test.tsx +++ b/src/components/servers/MCPServerDetailsPanel.test.tsx @@ -444,10 +444,38 @@ describe("MCPServerDetailsPanel", () => { ); await waitFor(() => { - expect(screen.getByText("Unreachable")).toBeInTheDocument(); + expect(screen.getByText("Offline")).toBeInTheDocument(); }); }); + it("shows OAuth authorization action from shared status", async () => { + const user = userEvent.setup(); + const onAuthorize = vi.fn().mockResolvedValue(undefined); + + renderWithProviders( + {}} + oauthStatus={{ + state: "ready", + tokenStatus: "missing", + status: { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "missing", authorized: false }, + }, + }} + onAuthorize={onAuthorize} + />, + ); + + await user.click(screen.getByRole("button", { name: "Authorize Test MCP Server" })); + + expect(onAuthorize).toHaveBeenCalledOnce(); + }); + it("closes panel when close button is clicked", async () => { const user = userEvent.setup(); const onClose = vi.fn(); diff --git a/src/components/servers/MCPServerDetailsPanel.tsx b/src/components/servers/MCPServerDetailsPanel.tsx index 5d06f4cb..3aa0e9cb 100644 --- a/src/components/servers/MCPServerDetailsPanel.tsx +++ b/src/components/servers/MCPServerDetailsPanel.tsx @@ -2,7 +2,6 @@ import { useCallback, useEffect, useMemo, useRef, useState, type KeyboardEvent } import type { ReactNode } from "react"; import { useIntl } from "react-intl"; import { - Activity, Box, Globe, Loader2, @@ -28,7 +27,9 @@ import { TruncatedMiddleText } from "@/components/ui/truncated-middle-text"; import { cn } from "@/lib/utils"; import type { MCPServer as BaseMCPServer, VirtualServerTag } from "@/types/server"; import { useQuery } from "@/hooks/useQuery"; +import type { OAuthStatusEntry } from "@/hooks/useOAuthStatuses"; import { TestConnectionPanel } from "./TestConnectionPanel"; +import { ServerStatusIndicator } from "./ServerStatusIndicator"; import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; interface MCPServer extends BaseMCPServer { @@ -135,6 +136,9 @@ export function MCPServerDetailsPanel({ onClose, initialTab = "tryit", onAddTag, + oauthStatus, + onAuthorize, + onRetryOAuthStatus, }: { server: MCPServer | null; error: { message: string } | null; @@ -148,6 +152,9 @@ export function MCPServerDetailsPanel({ * row shows a non-interactive "add" affordance. */ onAddTag?: (serverId: string, tags: string[]) => Promise; + oauthStatus?: OAuthStatusEntry; + onAuthorize?: () => Promise; + onRetryOAuthStatus?: () => void; }) { const [topTab, setTopTab] = useState(initialTab); const [activeTab, setActiveTab] = useState("all"); @@ -559,16 +566,12 @@ export function MCPServerDetailsPanel({
- - - {server.enabled ? (server.reachable ? "Active" : "Unreachable") : "Inactive"} - + diff --git a/src/components/servers/ServerStatusDetail.tsx b/src/components/servers/ServerStatusDetail.tsx index 6164d3d6..c97f9cce 100644 --- a/src/components/servers/ServerStatusDetail.tsx +++ b/src/components/servers/ServerStatusDetail.tsx @@ -1,42 +1,39 @@ import { useIntl } from "react-intl"; -import { getAvailabilityPresentation, type ServerAvailability } from "@/lib/serverStatus"; +import { Button } from "@/components/ui/button"; +import type { ServerAvailability } from "@/lib/serverStatus"; +import { getAvailabilityPresentation, needsOAuthAuthorization } from "@/lib/serverStatus"; import { formatLocalDateTime } from "@/utils/formatDate"; -interface ServerStatusDetailProps { - availability: ServerAvailability; - enabled: boolean; - lastSeen?: string | null; - lastError?: string | null; -} - -/** - * What a status means, plus the last response and last error where the server - * has them. This is the only place either value is surfaced in the UI. - * - * Disabled servers withhold the error: the health loop clears `last_error` only - * on enabled servers, so theirs is left over from an outage before they were - * turned off and reads as a current failure. The test is `enabled` rather than - * the `inactive` state, because `auth` outranks `inactive`, so a disabled - * server whose token has also expired is classified `auth` and would otherwise - * slip past the guard. - */ export function ServerStatusDetail({ availability, enabled, lastSeen, lastError, -}: ServerStatusDetailProps) { + onRetry, + authorizationManagementHint = false, +}: { + availability: ServerAvailability; + enabled: boolean; + lastSeen?: string | null; + lastError?: string | null; + onRetry?: () => void; + authorizationManagementHint?: boolean; +}) { const intl = useIntl(); + const presentation = getAvailabilityPresentation(availability); const showLastError = Boolean(lastError) && enabled; return (
-

- {intl.formatMessage({ id: getAvailabilityPresentation(availability).detailId })} -

- {lastSeen && ( +

{intl.formatMessage({ id: presentation.detailId })}

+ {authorizationManagementHint && needsOAuthAuthorization(availability) && (

+ {intl.formatMessage({ id: "mcpServer.status.detail.manageAuthorization" })} +

+ )} + {lastSeen && ( +

{intl.formatMessage( { id: "mcpServer.status.detail.lastSeen" }, { timestamp: formatLocalDateTime(lastSeen, "") }, @@ -48,6 +45,11 @@ export function ServerStatusDetail({ {intl.formatMessage({ id: "mcpServer.status.detail.lastError" }, { error: lastError })}

)} + {onRetry && ( + + )}
); } diff --git a/src/components/servers/ServerStatusIndicator.test.tsx b/src/components/servers/ServerStatusIndicator.test.tsx index 48eaebfa..9132b86a 100644 --- a/src/components/servers/ServerStatusIndicator.test.tsx +++ b/src/components/servers/ServerStatusIndicator.test.tsx @@ -2,9 +2,19 @@ import { describe, expect, it, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import { renderWithProviders, screen } from "@/test/test-utils"; +import type { OAuthTokenStatus } from "@/api/oauth"; import { ServerStatusIndicator } from "./ServerStatusIndicator"; const server = { name: "github-notify", enabled: true, reachable: true }; +const readyOAuthStatus = (tokenStatus: OAuthTokenStatus) => ({ + state: "ready" as const, + status: { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: tokenStatus, authorized: tokenStatus === "valid" }, + }, + tokenStatus, +}); describe("ServerStatusIndicator", () => { it("explains a state with nothing to resolve, in a popover", async () => { @@ -38,6 +48,20 @@ describe("ServerStatusIndicator", () => { expect(screen.queryByText(/Last response:/)).not.toBeInTheDocument(); }); + it("does not expose an invalid raw last-response timestamp", async () => { + const user = userEvent.setup(); + renderWithProviders( + , + ); + + await user.click(screen.getByRole("button", { name: /status: Offline/i })); + + expect(await screen.findByText(/Last response:/)).toBeInTheDocument(); + expect(screen.queryByText(/invalid-backend-timestamp/)).not.toBeInTheDocument(); + }); + it("withholds the stale last error an inactive server kept from its last outage", async () => { const user = userEvent.setup(); renderWithProviders( @@ -70,13 +94,13 @@ describe("ServerStatusIndicator", () => { lastSeen: "2026-04-16T13:23:12Z", lastError: "certificate has expired", }} - oauthTokenStatus="expired" + oauthStatus={readyOAuthStatus("expired")} />, ); - await user.click(screen.getByRole("button", { name: /status: Authorization/i })); + await user.click(screen.getByRole("button", { name: /status: Authorization expired/i })); - expect(await screen.findByText(/You have not authorized this server/)).toBeInTheDocument(); + expect(await screen.findByText(/authorization has expired/)).toBeInTheDocument(); expect(screen.queryByText(/certificate has expired/)).not.toBeInTheDocument(); }); @@ -86,7 +110,7 @@ describe("ServerStatusIndicator", () => { renderWithProviders( , ); @@ -99,9 +123,11 @@ describe("ServerStatusIndicator", () => { it("explains the auth state where the caller cannot authorize", async () => { const user = userEvent.setup(); - renderWithProviders(); + renderWithProviders( + , + ); - await user.click(screen.getByRole("button", { name: /status: Authorization/i })); + await user.click(screen.getByRole("button", { name: /status: Authorization required/i })); expect(await screen.findByText(/You have not authorized this server/)).toBeInTheDocument(); }); @@ -117,7 +143,7 @@ describe("ServerStatusIndicator", () => { renderWithProviders( , ); @@ -127,19 +153,21 @@ describe("ServerStatusIndicator", () => { const trigger = screen.getByRole("button", { name: "Authorize github-notify" }); expect(trigger).toBeDisabled(); expect(trigger).toHaveTextContent("Authorizing..."); - expect(screen.getByText("Authorization")).toHaveAttribute("aria-hidden", "true"); + expect(screen.getByText("Authorization required")).toHaveAttribute("aria-hidden", "true"); release?.(); }); it("announces the full status word where the label is abbreviated", () => { renderWithProviders( - , + , ); expect(screen.getByText("Auth")).toHaveAttribute("aria-hidden", "true"); expect( - screen.getByRole("button", { name: "github-notify status: Authorization. Show details" }), + screen.getByRole("button", { + name: "github-notify status: Authorization required. Show details", + }), ).toBeInTheDocument(); }); diff --git a/src/components/servers/ServerStatusIndicator.tsx b/src/components/servers/ServerStatusIndicator.tsx index 99cebf06..4342e0ba 100644 --- a/src/components/servers/ServerStatusIndicator.tsx +++ b/src/components/servers/ServerStatusIndicator.tsx @@ -2,89 +2,70 @@ import { useState } from "react"; import { useIntl } from "react-intl"; import { Popover, PopoverContent, PopoverTrigger } from "@/components/ui/popover"; -import { cn } from "@/lib/utils"; +import { isRetryableOAuthStatus, type OAuthStatusEntry } from "@/hooks/useOAuthStatuses"; import { getAvailabilityPresentation, getServerAvailability, - type OAuthTokenStatus, + needsOAuthAuthorization, type ServerAvailabilityInput, } from "@/lib/serverStatus"; +import { cn } from "@/lib/utils"; import { ServerStatusDetail } from "./ServerStatusDetail"; interface ServerStatusIndicatorProps { - server: ServerAvailabilityInput & { - name: string; - lastError?: string | null; - }; - oauthTokenStatus?: OAuthTokenStatus; - /** Use the short label, for narrow columns. Screen readers still get the full one. */ + server: ServerAvailabilityInput & { name: string; lastError?: string | null }; + oauthStatus?: OAuthStatusEntry; compact?: boolean; - /** Render as plain text rather than a button. Required inside another button. */ interactive?: boolean; - /** - * Starts the OAuth authorization flow. Given only where the caller can run - * it; without it the `auth` state explains itself like every other state. - */ onAuthorize?: () => Promise; + onRetry?: () => void; + authorizationManagementHint?: boolean; className?: string; } -/** - * Server status icon and label. - * - * States with nothing to resolve open a popover explaining themselves, the way - * visibility does. `auth` is the exception: it hands off to the OAuth flow - * rather than describing it, since authorizing is the whole point of the state. - * - * That button stacks its two labels so the wider one sets the width, keeping - * the row still while the flow is open. - */ export function ServerStatusIndicator({ server, - oauthTokenStatus, + oauthStatus, compact = false, interactive = true, onAuthorize, + onRetry, + authorizationManagementHint = false, className, }: ServerStatusIndicatorProps) { const intl = useIntl(); const [isAuthorizing, setIsAuthorizing] = useState(false); - const availability = getServerAvailability(server, oauthTokenStatus); + const availability = getServerAvailability(server, oauthStatus); const presentation = getAvailabilityPresentation(availability); const StatusIcon = presentation.Icon; - const authorize = availability === "auth" ? onAuthorize : undefined; + const authorize = needsOAuthAuthorization(availability) ? onAuthorize : undefined; + const canRetry = + availability === "authorization_unavailable" && isRetryableOAuthStatus(oauthStatus); const statusLabel = intl.formatMessage({ id: compact ? presentation.shortLabelId : presentation.labelId, }); - const authorizingLabel = intl.formatMessage({ id: "mcpServer.status.action.authorizing" }); + const authorizingLabel = intl.formatMessage({ id: "mcpServer.status.authorizing" }); const label = isAuthorizing ? authorizingLabel : statusLabel; const fullLabel = intl.formatMessage({ id: presentation.labelId }); const isAbbreviated = compact && presentation.shortLabelId !== presentation.labelId; - - const icon = ( -