diff --git a/.secrets.baseline b/.secrets.baseline index 6e0b4c1a..74e4224f 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -3,7 +3,7 @@ "files": "(?x)(package-lock\\.json$)|^\\.secrets\\.baseline$|src/i18n/locales/*|openapi.json|^.secrets.baseline$", "lines": null }, - "generated_at": "2026-11-02T13:55:29Z", + "generated_at": "2026-11-06T13:27:54Z", "plugins_used": [ { "name": "AWSKeyDetector" diff --git a/e2e/oauth-connection-validation.spec.ts b/e2e/oauth-connection-validation.spec.ts index 7bde38da..c0ed188a 100644 --- a/e2e/oauth-connection-validation.spec.ts +++ b/e2e/oauth-connection-validation.spec.ts @@ -1,162 +1,212 @@ import { test, expect } from "./fixtures/auth"; import { APP } from "./utils/paths"; -test.describe("Servers post-authorization connection validation", () => { - test("authorization exposes a manual test, scope failure can be reauthorized and retested", async ({ - page, - context, - apiMock, - }) => { - await apiMock.mockPermissions(); - let authorized = false; - let scopeFailure = true; - let tests = 0; - let imports = 0; - const gateway = { - id: "oauth-validation", - name: "Validation server", - url: "https://saved.example/mcp", - transport: "STREAMABLEHTTP", - enabled: true, - reachable: true, - visibility: "public", - authType: "oauth", - oauthConfig: { grant_type: "authorization_code" }, - createdAt: "2026-11-02T00:00:00Z", - updatedAt: "2026-11-02T00:00:00Z", - }; - await page.route("**/v1/mcp-servers?*", (route) => - route.fulfill({ json: { gateways: [gateway], nextCursor: null } }), - ); - await page.route(`**/v1/mcp-servers/${gateway.id}`, (route) => - route.fulfill({ json: gateway }), - ); - await page.route("**/oauth/status?*", (route) => - route.fulfill({ - json: { - [gateway.id]: { - oauth_enabled: true, - grant_type: "authorization_code", - user_token_status: { status: authorized ? "valid" : "missing", authorized }, +for (const catalog of [false, true]) { + const surface = catalog ? "Catalog" : "Servers"; + test.describe(`${surface} post-authorization connection validation`, () => { + test("authorization exposes a manual test, scope failure can be reauthorized and retested", async ({ + page, + context, + apiMock, + }) => { + await apiMock.mockPermissions(); + let authorized = false; + let scopeFailure = true; + let tests = 0; + let imports = 0; + const gateway = { + id: "oauth-validation", + name: "Validation server", + url: "https://saved.example/mcp", + transport: "STREAMABLEHTTP", + enabled: true, + reachable: true, + visibility: "public", + authType: "oauth", + oauthConfig: { grant_type: "authorization_code" }, + createdAt: "2026-11-02T00:00:00Z", + updatedAt: "2026-11-02T00:00:00Z", + }; + await page.route("**/v1/mcp-servers?*", (route) => + route.fulfill({ json: { gateways: [gateway], nextCursor: null } }), + ); + await page.route(`**/v1/mcp-servers/${gateway.id}`, (route) => + route.fulfill({ json: gateway }), + ); + await page.route("**/v1/catalog?*", (route) => + route.fulfill({ + json: { + servers: [ + { + id: "catalog-validation", + name: gateway.name, + url: "https://template.example/mcp", + description: "OAuth test server", + auth_type: "OAuth", + category: "Development", + provider: "Example", + is_registered: true, + gateway_id: gateway.id, + requires_oauth_config: false, + }, + ], + total: 1, }, - }, - }), - ); - await page.route("**/oauth/authorize-nonce", (route) => - route.fulfill({ json: { nonce: "validation-nonce" } }), - ); - await context.route("**/oauth/authorize/**", (route) => - route.fulfill({ contentType: "text/html", body: "Provider" }), - ); - await page.route(`**/oauth/fetch-tools/${gateway.id}`, (route) => { - imports++; - return route.fulfill({ json: { success: true } }); - }); - await page.route(`**/v1/mcp-servers/${gateway.id}/state?*`, (route) => - route.fulfill({ json: { status: "success" } }), - ); - await page.route("**/v1/mcp-servers/test-handshake", (route) => { - tests++; - expect(route.request().postDataJSON()).toEqual({ - baseUrl: gateway.url, - gatewayId: gateway.id, - validationMode: "post_authorization", + }), + ); + await page.route("**/oauth/status?*", (route) => + route.fulfill({ + json: { + [gateway.id]: { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: authorized ? "valid" : "missing", authorized }, + }, + }, + }), + ); + await page.route("**/oauth/authorize-nonce", (route) => + route.fulfill({ json: { nonce: "validation-nonce" } }), + ); + await context.route("**/oauth/authorize/**", (route) => + route.fulfill({ contentType: "text/html", body: "Provider" }), + ); + await page.route(`**/oauth/fetch-tools/${gateway.id}`, (route) => { + imports++; + return route.fulfill({ json: { success: true } }); }); - return route.fulfill({ - json: { - success: !scopeFailure, - latencyMs: 3, - stages: { token: "ok", negotiation: "ok", tools_list: scopeFailure ? "failed" : "ok" }, - errorCode: scopeFailure ? "insufficient_scope" : null, - reauthorizationRequired: scopeFailure, - componentCounts: scopeFailure ? null : { tools: 0 }, - }, + await page.route(`**/v1/mcp-servers/${gateway.id}/state?*`, (route) => + route.fulfill({ json: { status: "success" } }), + ); + await page.route("**/v1/mcp-servers/test-handshake", (route) => { + tests++; + expect(route.request().postDataJSON()).toEqual({ + baseUrl: gateway.url, + gatewayId: gateway.id, + validationMode: "post_authorization", + }); + return route.fulfill({ + json: { + success: !scopeFailure, + latencyMs: 3, + stages: { token: "ok", negotiation: "ok", tools_list: scopeFailure ? "failed" : "ok" }, + errorCode: scopeFailure ? "insufficient_scope" : null, + reauthorizationRequired: scopeFailure, + componentCounts: scopeFailure ? null : { tools: 0 }, + }, + }); }); + await page.goto(catalog ? APP.SERVER_CATALOG : APP.SERVERS); + const popupPromise = context.waitForEvent("page"); + if (catalog) { + await page.getByRole("button", { name: "Actions for Validation server" }).click(); + await page.getByRole("menuitem", { name: "Authorize", exact: true }).click(); + } + if (!catalog) await page.getByRole("button", { name: "Authorize Validation server" }).click(); + const popup = await popupPromise; + await popup.waitForURL(/\/oauth\/authorize\//); + authorized = true; + await popup.evaluate(() => { + window.opener.postMessage({ type: "oauth_callback", status: "success" }, "*"); + window.close(); + }); + const button = page.getByRole("button", { + name: /^(Test connection|Retest) Validation server$/, + }); + await expect(button).toBeVisible(); + expect(tests).toBe(0); + await expect.poll(() => imports).toBe(1); + await button.click(); + await expect( + page.getByText( + "The stored token lacks the required scope. Review the scopes and authorize again.", + ), + ).toBeVisible(); + expect(imports).toBe(1); + const retryPopupPromise = context.waitForEvent("page"); + await page.getByRole("button", { name: "Authorize again" }).click(); + const retryPopup = await retryPopupPromise; + await retryPopup.waitForURL(/\/oauth\/authorize\//); + await expect(page.getByText("Connection validation failed")).toHaveCount(0); + await retryPopup.evaluate(() => { + window.opener.postMessage({ type: "oauth_callback", status: "success" }, "*"); + window.close(); + }); + await expect(button).toBeVisible(); + scopeFailure = false; + await button.click(); + await expect(page.getByText("Stored authorization verified")).toBeVisible(); + await expect(page.getByText("Tools access: Passed")).toBeVisible(); + await expect(page.getByRole("status").getByText("0 tools", { exact: true })).toBeVisible(); + expect(tests).toBe(2); }); - await page.goto(APP.SERVERS); - const popupPromise = context.waitForEvent("page"); - await page.getByRole("button", { name: "Authorize Validation server" }).click(); - const popup = await popupPromise; - await popup.waitForURL(/\/oauth\/authorize\//); - authorized = true; - await popup.evaluate(() => { - window.opener.postMessage({ type: "oauth_callback", status: "success" }, "*"); - window.close(); - }); - const button = page.getByRole("button", { - name: /^(Test connection|Retest) Validation server$/, - }); - await expect(button).toBeVisible(); - expect(tests).toBe(0); - await expect.poll(() => imports).toBe(1); - await button.click(); - await expect( - page.getByText( - "The stored token lacks the required scope. Review the scopes and authorize again.", - ), - ).toBeVisible(); - expect(imports).toBe(1); - const retryPopupPromise = context.waitForEvent("page"); - await page.getByRole("button", { name: "Authorize again" }).click(); - const retryPopup = await retryPopupPromise; - await retryPopup.waitForURL(/\/oauth\/authorize\//); - await expect(page.getByText("Connection validation failed")).toHaveCount(0); - await retryPopup.evaluate(() => { - window.opener.postMessage({ type: "oauth_callback", status: "success" }, "*"); - window.close(); - }); - await expect(button).toBeVisible(); - scopeFailure = false; - await button.click(); - await expect(page.getByText("Stored authorization verified")).toBeVisible(); - await expect(page.getByText("Tools access: Passed")).toBeVisible(); - await expect(page.getByRole("status").getByText("0 tools", { exact: true })).toBeVisible(); - expect(tests).toBe(2); - }); - test("read-only users cannot run stored-token validation", async ({ page, apiMock }) => { - await apiMock.mockPermissions({ permissions: ["gateways.read"] }); - const gateway = { - id: "read-only-oauth", - name: "Read only OAuth", - url: "https://saved.example/mcp", - enabled: true, - reachable: true, - authType: "oauth", - oauthConfig: { grant_type: "authorization_code" }, - visibility: "public", - }; - await page.route("**/v1/mcp-servers?*", (route) => - route.fulfill({ json: { gateways: [gateway] } }), - ); - await page.route(`**/v1/mcp-servers/${gateway.id}`, (route) => - route.fulfill({ json: gateway }), - ); - await page.route("**/oauth/status?*", (route) => - route.fulfill({ - json: { - [gateway.id]: { - oauth_enabled: true, - grant_type: "authorization_code", - user_token_status: { status: "valid", authorized: true }, + test("read-only users cannot run stored-token validation", async ({ page, apiMock }) => { + await apiMock.mockPermissions({ permissions: ["gateways.read"] }); + const gateway = { + id: "read-only-oauth", + name: "Read only OAuth", + url: "https://saved.example/mcp", + enabled: true, + reachable: true, + authType: "oauth", + oauthConfig: { grant_type: "authorization_code" }, + visibility: "public", + }; + await page.route("**/v1/mcp-servers?*", (route) => + route.fulfill({ json: { gateways: [gateway] } }), + ); + await page.route(`**/v1/mcp-servers/${gateway.id}`, (route) => + route.fulfill({ json: gateway }), + ); + await page.route("**/v1/catalog?*", (route) => + route.fulfill({ + json: { + servers: [ + { + id: "read-only", + name: gateway.name, + url: gateway.url, + auth_type: "OAuth", + is_registered: true, + gateway_id: gateway.id, + }, + ], + total: 1, }, - }, - }), - ); - const statusResponse = page.waitForResponse( - (response) => response.url().includes("/oauth/status?") && response.ok(), - ); - await page.goto(APP.SERVERS); - await statusResponse; - await expect(page.getByText(gateway.name, { exact: true })).toBeVisible(); - await expect( - page.getByRole("button", { name: `${gateway.name} status: Active. Show details` }), - ).toBeVisible(); - await expect( - page.getByRole("button", { name: "Test connection Read only OAuth", exact: true }), - ).toHaveCount(0); + }), + ); + await page.route("**/oauth/status?*", (route) => + route.fulfill({ + json: { + [gateway.id]: { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "valid", authorized: true }, + }, + }, + }), + ); + const statusResponse = page.waitForResponse( + (response) => response.url().includes("/oauth/status?") && response.ok(), + ); + await page.goto(catalog ? APP.SERVER_CATALOG : APP.SERVERS); + await statusResponse; + await expect(page.getByText(gateway.name, { exact: true })).toBeVisible(); + if (!catalog) + await expect( + page.getByRole("button", { name: `${gateway.name} status: Active. Show details` }), + ).toBeVisible(); + await expect( + page.getByRole("button", { name: "Test connection Read only OAuth", exact: true }), + ).toHaveCount(0); + if (catalog) { + await page.getByRole("button", { name: `Actions for ${gateway.name}` }).click(); + await expect(page.getByRole("menuitem", { name: "Test connection" })).toHaveCount(0); + } + }); }); -}); +} for (const [code, guidance] of [ [ diff --git a/src/components/server-catalog/CatalogResults.test.tsx b/src/components/server-catalog/CatalogResults.test.tsx index 5505de43..d32d5b13 100644 --- a/src/components/server-catalog/CatalogResults.test.tsx +++ b/src/components/server-catalog/CatalogResults.test.tsx @@ -36,7 +36,7 @@ function catalogResults( onDisconnect={vi.fn()} testingServerIds={testingServerIds} disconnectingServerIds={disconnectingServerIds} - canTest={false} + getTestAction={() => ({ kind: "generic", availability: "hidden" })} canDisconnect={false} /> ); @@ -136,7 +136,7 @@ describe("CatalogResults", () => { onTest={vi.fn()} onAuthorize={onAuthorize} onDisconnect={vi.fn()} - canTest={false} + getTestAction={() => ({ kind: "generic", availability: "hidden" })} canDisconnect={false} oauthStatuses={{ "gateway-github": { @@ -185,7 +185,7 @@ describe("CatalogResults", () => { onDisconnect={onDisconnect} testingServerIds={new Set()} disconnectingServerIds={new Set()} - canTest + getTestAction={() => ({ kind: "oauth", availability: "disabled" })} canDisconnect oauthStatuses={{ "gateway-github": { @@ -245,7 +245,7 @@ describe("CatalogResults", () => { onTest={vi.fn()} onAuthorize={vi.fn()} onDisconnect={vi.fn()} - canTest={false} + getTestAction={() => ({ kind: "generic", availability: "hidden" })} canDisconnect={false} oauthStatuses={{ "gateway-valid": { @@ -296,7 +296,7 @@ describe("CatalogResults", () => { onTest={vi.fn()} onAuthorize={onAuthorize} onDisconnect={vi.fn()} - canTest={false} + getTestAction={() => ({ kind: "generic", availability: "hidden" })} canDisconnect={false} />, ); @@ -326,7 +326,7 @@ describe("CatalogResults", () => { onTest={vi.fn()} onAuthorize={vi.fn()} onDisconnect={vi.fn()} - canTest={false} + getTestAction={() => ({ kind: "generic", availability: "hidden" })} canDisconnect={false} />, ); diff --git a/src/components/server-catalog/CatalogResults.tsx b/src/components/server-catalog/CatalogResults.tsx index d272d164..17f51147 100644 --- a/src/components/server-catalog/CatalogResults.tsx +++ b/src/components/server-catalog/CatalogResults.tsx @@ -25,6 +25,7 @@ import { } from "@/components/ui/dropdown-menu"; import { StatusIndicator } from "@/components/ui/status-indicator"; import type { CatalogServer } from "@/generated/types"; +import type { CatalogTestAction } from "@/lib/catalogConnectionValidation"; import { useDebouncedValue } from "@/hooks/useDebouncedValue"; import { getAuthTypeGroupId, @@ -118,12 +119,13 @@ function CatalogCard({ onView, onAdd, onTest, + getTestAction, + renderOAuthValidation, onAuthorize, onDisconnect, isAdding, isTesting, isDisconnecting, - canTest, canDisconnect, oauthStatuses, addError, @@ -134,12 +136,13 @@ function CatalogCard({ onView: (trigger: HTMLElement) => void; onAdd: () => void; onTest: () => void; + getTestAction: (server: CatalogServer) => CatalogTestAction; + renderOAuthValidation?: (server: CatalogServer) => ReactNode; onAuthorize: () => void; onDisconnect: () => void; isAdding: boolean; isTesting: boolean; isDisconnecting: boolean; - canTest: boolean; canDisconnect: boolean; oauthStatuses?: Readonly>; /** Why the last add attempt failed, resolved to the server's reason or the fallback. */ @@ -163,6 +166,7 @@ function CatalogCard({ server, server.gateway_id ? oauthStatuses?.[server.gateway_id] : undefined, ); + const testAction = getTestAction(server); const authTypeGroupId = getAuthTypeGroupId(server.auth_type); const requiresAuthentication = authTypeGroupId === "apiKey" || authTypeGroupId === "oauth"; const StatusIcon = STATUS_ICON[oauthState.severity]; @@ -279,9 +283,9 @@ function CatalogCard({ > {intl.formatMessage({ id: "mcpServer.catalog.viewDetails" })} - {canTest && ( + {testAction.availability !== "hidden" && ( )} + {renderOAuthValidation?.(server)} @@ -481,11 +486,12 @@ export function CatalogResults({ onAdd, addingServerIds, onTest, + getTestAction, + renderOAuthValidation, onAuthorize, onDisconnect, testingServerIds = EMPTY_PENDING_IDS, disconnectingServerIds = EMPTY_PENDING_IDS, - canTest, canDisconnect, oauthStatuses = EMPTY_OAUTH_STATUSES, addErrors = EMPTY_ADD_ERRORS, @@ -498,11 +504,12 @@ export function CatalogResults({ onAdd: (server: CatalogServer) => void; addingServerIds: ReadonlySet; onTest: (server: CatalogServer) => void; + getTestAction: (server: CatalogServer) => CatalogTestAction; + renderOAuthValidation?: (server: CatalogServer) => ReactNode; onAuthorize: (server: CatalogServer) => void; onDisconnect: (server: CatalogServer) => void; testingServerIds?: ReadonlySet; disconnectingServerIds?: ReadonlySet; - canTest: boolean; canDisconnect: boolean; oauthStatuses?: Readonly>; addErrors?: Readonly>; @@ -529,12 +536,13 @@ export function CatalogResults({ onView={(trigger) => onView(server, trigger)} onAdd={() => onAdd(server)} onTest={() => onTest(server)} + getTestAction={getTestAction} + renderOAuthValidation={renderOAuthValidation} onAuthorize={() => onAuthorize(server)} onDisconnect={() => onDisconnect(server)} isAdding={addingServerIds.has(server.id)} isTesting={testingServerIds.has(server.id)} isDisconnecting={disconnectingServerIds.has(server.id)} - canTest={canTest} canDisconnect={canDisconnect} oauthStatuses={oauthStatuses} addError={addErrors[server.id]} diff --git a/src/lib/catalogConnectionValidation.test.ts b/src/lib/catalogConnectionValidation.test.ts new file mode 100644 index 00000000..9a52cfd8 --- /dev/null +++ b/src/lib/catalogConnectionValidation.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vitest"; +import type { CatalogServer } from "@/generated/types"; +import { catalogTestAction } from "./catalogConnectionValidation"; + +const server = { auth_type: "OAuth", is_registered: true, gateway_id: "gateway" } as CatalogServer; +const options = { canTest: true, canUpdate: true, canValidate: true, pending: false }; + +describe("Catalog connection test policy", () => { + it.each([ + ["no read permission", { canTest: false }, "hidden"], + ["read-only OAuth caller", { canUpdate: false }, "hidden"], + ["ineligible gateway or token", { canValidate: false }, "disabled"], + ["another action pending", { pending: true }, "disabled"], + ["eligible gateway", {}, "enabled"], + ] as const)("handles %s", (_name, changes, availability) => { + expect(catalogTestAction(server, { ...options, ...changes })).toEqual({ + kind: "oauth", + availability, + }); + }); + + it("keeps client-credentials OAuth on the generic test without update permission", () => { + expect( + catalogTestAction(server, { + ...options, + canUpdate: false, + canValidate: false, + status: { state: "not_applicable", status: { oauth_enabled: true } }, + }), + ).toEqual({ kind: "generic", availability: "enabled" }); + }); + + it("keeps non-OAuth tests available independently of OAuth eligibility", () => { + expect( + catalogTestAction( + { ...server, auth_type: "Open" }, + { + ...options, + canUpdate: false, + canValidate: false, + }, + ), + ).toEqual({ kind: "generic", availability: "enabled" }); + }); + + it("disables tests until required OAuth configuration is saved", () => { + expect( + catalogTestAction({ ...server, requires_oauth_config: true }, options).availability, + ).toBe("disabled"); + }); +}); diff --git a/src/lib/catalogConnectionValidation.ts b/src/lib/catalogConnectionValidation.ts new file mode 100644 index 00000000..b43f6628 --- /dev/null +++ b/src/lib/catalogConnectionValidation.ts @@ -0,0 +1,34 @@ +import type { CatalogServer } from "@/generated/types"; +import type { OAuthStatusEntry } from "@/api/oauth"; +import { isCatalogOAuthServer } from "@/utils/catalogAuthTypes"; + +export interface CatalogTestAction { + kind: "oauth" | "generic"; + availability: "hidden" | "disabled" | "enabled"; +} + +// Keep menu visibility, disabled state and click handling on the same policy. +export function catalogTestAction( + server: CatalogServer, + options: { + status?: OAuthStatusEntry; + canTest: boolean; + canUpdate: boolean; + canValidate: boolean; + pending: boolean; + }, +): CatalogTestAction { + const kind = + isCatalogOAuthServer(server) && options.status?.state !== "not_applicable" + ? "oauth" + : "generic"; + const availability = + !options.canTest || (kind === "oauth" && !options.canUpdate) + ? "hidden" + : server.requires_oauth_config || + options.pending || + (kind === "oauth" && !options.canValidate) + ? "disabled" + : "enabled"; + return { kind, availability }; +} diff --git a/src/lib/oauthConnectionValidation.test.ts b/src/lib/oauthConnectionValidation.test.ts index ee54aef9..75be12eb 100644 --- a/src/lib/oauthConnectionValidation.test.ts +++ b/src/lib/oauthConnectionValidation.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect } from "vitest"; +import { OAuthAuthorizationError } from "@/api/servers"; import type { MCPServer } from "@/types/server"; +import type { OAuthStatusEntry } from "@/api/oauth"; import { createIntl, createIntlCache } from "react-intl"; import en from "@/i18n/locales/en-US/mcpServer.json"; import es from "@/i18n/locales/es-ES/mcpServer.json"; @@ -8,6 +10,7 @@ import { canValidateOAuthConnection, oauthConfigurationKey, oauthErrorMessage, + oauthAuthorizationErrorMessage, } from "./oauthConnectionValidation"; const gateway = { @@ -45,6 +48,20 @@ describe("OAuth validation eligibility and configuration", () => { ).toBe(false); expect(canValidateOAuthConnection(gateway)).toBe(false); }); + + it.each([ + ["valid", true], + ["near_expiry", true], + ["expired", false], + ["missing", false], + ] as const)("allows stored-token validation for %s: %s", (tokenStatus, expected) => { + const status: OAuthStatusEntry = { + state: "ready", + tokenStatus, + status: { oauth_enabled: true }, + }; + expect(canValidateOAuthConnection({ ...gateway, authType: "OAuth" }, status)).toBe(expected); + }); }); describe("OAuth error guidance", () => { @@ -83,3 +100,29 @@ describe("OAuth error guidance", () => { }, ); }); + +describe("OAuth authorization error details", () => { + const intl = createIntl({ locale: "en-US", messages: en }, createIntlCache()); + + it.each(["access_denied", "unlisted_provider_error"])( + "preserves provider context for %s with localized guidance", + (code) => { + const error = new OAuthAuthorizationError(code, "Contact your organization administrator"); + expect(oauthAuthorizationErrorMessage(intl, error)).toBe( + `${oauthErrorMessage(intl, code)} Provider message: Contact your organization administrator`, + ); + }, + ); + + it("omits empty and duplicate descriptions", () => { + const guidance = oauthErrorMessage(intl, "access_denied"); + for (const description of [undefined, "", guidance]) { + expect( + oauthAuthorizationErrorMessage( + intl, + new OAuthAuthorizationError("access_denied", description), + ), + ).toBe(guidance); + } + }); +}); diff --git a/src/lib/oauthConnectionValidation.ts b/src/lib/oauthConnectionValidation.ts index ba56ab3e..eb44fb60 100644 --- a/src/lib/oauthConnectionValidation.ts +++ b/src/lib/oauthConnectionValidation.ts @@ -56,6 +56,19 @@ export function oauthErrorMessage(intl: IntlShape, code?: string): string { }); } +export function oauthAuthorizationErrorMessage( + intl: IntlShape, + error: { errorCode?: string; description?: string }, +): string { + const guidance = oauthErrorMessage(intl, error.errorCode); + return error.description && error.description !== guidance + ? intl.formatMessage( + { id: "mcpServer.oauthValidation.providerError" }, + { guidance, description: error.description }, + ) + : guidance; +} + export function hasValidationStages(result: OAuthConnectionValidationResponse): boolean { return VALIDATION_STAGES.every((stage) => ["ok", "failed", "skipped", "unsupported"].includes(result?.stages?.[stage] ?? ""), diff --git a/src/pages/ServerCatalog.test.tsx b/src/pages/ServerCatalog.test.tsx index f18a5b67..81062adf 100644 --- a/src/pages/ServerCatalog.test.tsx +++ b/src/pages/ServerCatalog.test.tsx @@ -1,5 +1,5 @@ import type { ReactElement } from "react"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { act, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; @@ -9,7 +9,7 @@ import { registerCatalogServer, testCatalogServer, } from "@/api/catalog"; -import { ApiError } from "@/api/client"; +import { api, ApiError } from "@/api/client"; import { getOAuthStatuses } from "@/api/oauth"; import { OAuthAuthorizationError, @@ -58,32 +58,10 @@ vi.mock("@/api/oauth", async (importOriginal) => ({ ...(await importOriginal()), getOAuthStatuses: vi.fn(), })); -vi.mock("@/api/servers", () => ({ - // Real error classes so `instanceof` checks in the page keep working - // against rejections created from the mocked module surface. - OAuthAuthorizationError: class OAuthAuthorizationError extends Error { - code?: string; - description?: string; - constructor(code?: string, description?: string) { - super(description || code || "OAuth authorization failed"); - this.name = "OAuthAuthorizationError"; - this.code = code; - this.description = description; - } - }, - OAuthCancelledError: class OAuthCancelledError extends Error { - constructor(message = "OAuth authorization was cancelled") { - super(message); - this.name = "OAuthCancelledError"; - } - }, - OAuthPopupBlockedError: class OAuthPopupBlockedError extends Error { - constructor() { - super("Failed to open OAuth authorization window"); - this.name = "OAuthPopupBlockedError"; - } - }, +vi.mock("@/api/servers", async (importOriginal) => ({ + ...(await importOriginal()), serversApi: { + testOAuthConnection: vi.fn(), openOAuthAuthorizationPopup: vi.fn(), triggerOAuthAuthorization: vi.fn(), toggleEnabled: vi.fn(), @@ -217,6 +195,7 @@ function renderWithRouter(ui: ReactElement, path = "/app/server-catalog") { } describe("ServerCatalog", () => { + afterEach(() => vi.restoreAllMocks()); beforeEach(() => { window.history.replaceState({}, "", "/app/"); mockUseQuery.mockImplementation((path) => @@ -250,12 +229,178 @@ describe("ServerCatalog", () => { mockTriggerOAuthAuthorization.mockReset(); mockToggleEnabled.mockReset(); mockFetchToolsAfterOAuth.mockReset(); + vi.mocked(serversApi.testOAuthConnection).mockReset(); mockOpenOAuthAuthorizationPopup.mockReturnValue({ close: vi.fn() } as unknown as Window); mockTriggerOAuthAuthorization.mockResolvedValue({ type: "oauth_callback", status: "success" }); mockToggleEnabled.mockResolvedValue({ status: "success", message: "Activated" }); mockFetchToolsAfterOAuth.mockResolvedValue({ success: true, message: "Tools fetched" }); }); + it.each([ + ["oauth", true], + ["OAuth", true], + ["oauth", false], + ])("validates the freshly saved %s gateway only when enabled=%s", async (authType, enabled) => { + const user = userEvent.setup(); + const catalogServer = { + ...oauthServer, + is_registered: true, + gateway_id: "gateway-github", + requires_oauth_config: false, + }; + mockUseQuery.mockReturnValue( + queryResult({ data: { ...response, servers: [catalogServer], total: 1 } }), + ); + mockGetOAuthStatuses.mockResolvedValue({ + statuses: { + "gateway-github": { + oauth_enabled: true, + grant_type: "authorization_code", + user_token_status: { status: "valid", authorized: true }, + }, + }, + failures: {}, + }); + const gateway = { + id: "gateway-github", + name: "GitHub", + url: "https://saved.example/mcp", + enabled: true, + authType, + oauthConfig: { grant_type: "authorization_code" }, + }; + const savedGateway = deferred(); + const validation = deferred>>(); + const get = vi + .spyOn(api, "get") + .mockReturnValueOnce(savedGateway.promise) + .mockResolvedValue({ ...gateway, enabled }); + const testConnection = vi.spyOn(serversApi, "testOAuthConnection"); + testConnection.mockReturnValue(validation.promise); + renderWithRouter(); + await act(async () => savedGateway.resolve(gateway)); + const button = screen.getByRole("button", { name: "Test connection GitHub" }); + expect(testConnection).not.toHaveBeenCalled(); + await user.click(button); + if (!enabled) { + expect( + await screen.findByText( + "The saved server is not eligible for this test or the request is invalid. Reload and check its configuration.", + ), + ).toBeVisible(); + expect(testConnection).not.toHaveBeenCalled(); + return; + } + expect(get).toHaveBeenCalledTimes(2); + expect(testConnection).toHaveBeenCalledExactlyOnceWith( + { gatewayId: "gateway-github", baseUrl: gateway.url }, + expect.any(AbortSignal), + ); + expect( + screen.getByRole("button", { name: "Testing stored authorization… GitHub" }), + ).toHaveAttribute("aria-disabled", "true"); + await act(async () => { + validation.resolve({ + success: true, + latencyMs: 1, + stages: { token: "ok", negotiation: "ok", tools_list: "ok" }, + componentCounts: { tools: 0 }, + }); + }); + expect(screen.getByText("Stored authorization verified")).toBeVisible(); + expect(mockFetchToolsAfterOAuth).not.toHaveBeenCalled(); + expect(mockToggleEnabled).not.toHaveBeenCalled(); + expect(mockRegisterCatalogServer).not.toHaveBeenCalled(); + expect(mockTestCatalogServer).not.toHaveBeenCalled(); + }); + + it("does not expose OAuth Test to read-only callers", async () => { + const user = userEvent.setup(); + const catalogServer = { ...oauthServer, is_registered: true, gateway_id: "gateway-github" }; + mockUseQuery.mockReturnValue( + queryResult({ data: { ...response, servers: [catalogServer], total: 1 } }), + ); + authState.hasPermission.mockImplementation((permission) => permission === "gateways.read"); + renderWithRouter(); + await user.click(await screen.findByRole("button", { name: "Actions for GitHub" })); + expect(screen.queryByRole("menuitem", { name: "Test connection" })).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Test connection GitHub" }), + ).not.toBeInTheDocument(); + }); + + it("reuses cached and pending gateway requests when the catalog ID list changes", async () => { + const first = { ...oauthServer, is_registered: true, gateway_id: "gateway-first" }; + const second = { ...first, id: "second", name: "Second", gateway_id: "gateway-second" }; + const third = { ...first, id: "third", name: "Third", gateway_id: "gateway-third" }; + const gateway = { id: first.gateway_id, enabled: true, authType: "oauth" }; + const pending = deferred(); + const secondRequest = deferred(); + const get = vi + .spyOn(api, "get") + .mockReturnValueOnce(pending.promise) + .mockReturnValueOnce(secondRequest.promise); + const show = (servers: CatalogServer[]) => + mockUseQuery.mockReturnValue(queryResult({ data: { ...response, servers } })); + + show([first]); + const { rerender, unmount } = renderWithRouter(); + expect(get).toHaveBeenCalledTimes(1); + const firstSignal = get.mock.calls[0][2] as AbortSignal; + + show([first, second]); + rerender(withProviders()); + expect(get).toHaveBeenCalledTimes(2); + expect(firstSignal.aborted).toBe(false); + + await act(async () => pending.resolve(gateway)); + expect(get).toHaveBeenCalledTimes(2); + await act(async () => secondRequest.resolve({ ...gateway, id: second.gateway_id })); + show([second]); + rerender(withProviders()); + show([first, second]); + rerender(withProviders()); + expect(get).toHaveBeenCalledTimes(2); + + const lastRequest = deferred(); + get.mockReturnValue(lastRequest.promise); + show([first, second, third]); + rerender(withProviders()); + expect(get).toHaveBeenCalledTimes(3); + const thirdSignal = get.mock.calls[2][2] as AbortSignal; + expect(thirdSignal.aborted).toBe(false); + unmount(); + expect(thirdSignal.aborted).toBe(true); + await act(async () => lastRequest.resolve({ ...gateway, id: third.gateway_id })); + }); + + it("clears cached gateways and aborts pending requests when read permission is revoked", async () => { + const first = { ...oauthServer, is_registered: true, gateway_id: "gateway-first" }; + const second = { ...first, id: "second", name: "Second", gateway_id: "gateway-second" }; + const gateway = { id: first.gateway_id, enabled: true, authType: "oauth" }; + const firstRequest = deferred(); + const pending = deferred(); + const get = vi + .spyOn(api, "get") + .mockReturnValueOnce(firstRequest.promise) + .mockReturnValue(pending.promise); + mockUseQuery.mockReturnValue(queryResult({ data: { ...response, servers: [first, second] } })); + const { rerender } = renderWithRouter(); + await act(async () => firstRequest.resolve(gateway)); + expect(get).toHaveBeenCalledTimes(2); + const signal = get.mock.calls[1][2] as AbortSignal; + authState.hasPermission.mockReturnValue(false); + rerender(withProviders()); + expect(signal.aborted).toBe(true); + await act(async () => pending.resolve({ ...gateway, id: second.gateway_id })); + const reloaded = deferred(); + get.mockReturnValue(reloaded.promise); + authState.hasPermission.mockReturnValue(true); + rerender(withProviders()); + expect(get).toHaveBeenCalledTimes(4); + await act(async () => reloaded.resolve(gateway)); + }); + it("uses the catalog GET endpoint and shared loader", () => { mockUseQuery.mockReturnValue(queryResult({ data: undefined, isLoading: true })); @@ -646,7 +791,7 @@ describe("ServerCatalog", () => { expect(screen.getByRole("button", { name: "Actions for GitHub" })).toHaveFocus(); }); - it("shows the provider's reason in the dialog when authorization fails", async () => { + it("shows localized guidance with the provider description", async () => { const user = userEvent.setup(); mockTriggerOAuthAuthorization.mockRejectedValue( new OAuthAuthorizationError("access_denied", "Provider denied the request"), @@ -669,9 +814,11 @@ describe("ServerCatalog", () => { ); await user.click(within(form).getByRole("button", { name: "Save" })); - // Real failures still surface so the user can react; only the - // deliberate popup dismissal is silent. - expect(await within(form).findByText("Provider denied the request")).toBeVisible(); + expect( + await within(form).findByText( + "The provider denied authorization. Authorize this server again. Provider message: Provider denied the request", + ), + ).toBeVisible(); expect(within(form).getByRole("button", { name: "Save" })).toBeInTheDocument(); }); @@ -697,7 +844,9 @@ describe("ServerCatalog", () => { await user.click(within(form).getByRole("button", { name: "Save" })); expect( - await within(form).findByText("OAuth authorization could not be completed. Try again."), + await within(form).findByText( + "OAuth connection failed. Check the configuration and try again.", + ), ).toBeVisible(); expect(within(form).queryByText("OAuth authorization failed")).not.toBeInTheDocument(); expect(within(form).getByRole("button", { name: "Save" })).toBeInTheDocument(); @@ -725,7 +874,9 @@ describe("ServerCatalog", () => { await user.click(within(form).getByRole("button", { name: "Save" })); expect( - await within(form).findByText("OAuth authorization could not be completed. Try again."), + await within(form).findByText( + "The provider denied authorization. Authorize this server again.", + ), ).toBeVisible(); expect(within(form).queryByText("OAuth authorization failed")).not.toBeInTheDocument(); expect(within(form).getByRole("button", { name: "Save" })).toBeInTheDocument(); diff --git a/src/pages/ServerCatalog.tsx b/src/pages/ServerCatalog.tsx index 570a007b..7640669c 100644 --- a/src/pages/ServerCatalog.tsx +++ b/src/pages/ServerCatalog.tsx @@ -9,7 +9,7 @@ import { testCatalogServer, type GatewayImpactPreview, } from "@/api/catalog"; -import { ApiError } from "@/api/client"; +import { api, ApiError } from "@/api/client"; import { OAuthAuthorizationError, OAuthCancelledError, @@ -38,6 +38,15 @@ import type { CatalogServerRegisterBody, } from "@/generated/types"; import { useDebouncedValue } from "@/hooks/useDebouncedValue"; +import type { MCPServer } from "@/types/server"; +import { useOAuthConnectionValidation } from "@/hooks/useOAuthConnectionValidation"; +import { OAuthConnectionValidationPanel } from "@/components/servers/OAuthConnectionValidationPanel"; +import { + canValidateOAuthConnection, + oauthConfigurationKey, + oauthAuthorizationErrorMessage, +} from "@/lib/oauthConnectionValidation"; +import { catalogTestAction } from "@/lib/catalogConnectionValidation"; import { useOAuthStatuses } from "@/hooks/useOAuthStatuses"; import { useQuery } from "@/hooks/useQuery"; import { useRouter } from "@/router"; @@ -358,17 +367,15 @@ function CatalogPageLayout({ ); } -// Surface the provider's own description when it sent one. A blocked popup -// gets its own message (retrying won't help). Everything else, including a -// provider denial with only a raw error code, gets the localized text. -function oauthErrorMessage(intl: IntlShape, error: unknown): string { +// Preserve popup-blocker guidance and use safe localized provider error mappings. +function catalogOAuthErrorMessage(intl: IntlShape, error: unknown): string { if (error instanceof OAuthPopupBlockedError) { return intl.formatMessage({ id: "mcpServer.catalog.oauth.popupBlocked" }); } - return ( - (error instanceof OAuthAuthorizationError && error.description) || - intl.formatMessage({ id: "mcpServer.catalog.oauth.authorizationError" }) - ); + if (error instanceof OAuthAuthorizationError) { + return oauthAuthorizationErrorMessage(intl, error); + } + return intl.formatMessage({ id: "mcpServer.catalog.oauth.authorizationError" }); } export function ServerCatalog() { @@ -437,7 +444,86 @@ export function ServerCatalog() { reload: reloadOAuthStatuses, retry: retryOAuthStatus, } = useOAuthStatuses(oauthGatewayIds, { enabled: canReadOAuthStatuses }); - const canTest = canReadOAuthStatuses; + const [savedOAuthGateways, setSavedOAuthGateways] = useState>({}); + const gatewayRequests = useRef(new Map()); + const abortGatewayRequests = useCallback(() => { + gatewayRequests.current.forEach((controller) => controller.abort()); + gatewayRequests.current.clear(); + }, []); + useEffect(() => abortGatewayRequests, [abortGatewayRequests]); + const oauthGatewayIdsKey = JSON.stringify(oauthGatewayIds); + useEffect(() => { + if (!canReadOAuthStatuses) { + abortGatewayRequests(); + setSavedOAuthGateways((previous) => (Object.keys(previous).length ? {} : previous)); + return; + } + const ids = JSON.parse(oauthGatewayIdsKey) as string[]; + // Keep cached and in-flight entries across ID-list changes. Explicit tests + // and authorization refreshes still fetch the current saved configuration. + ids.forEach((id) => { + if (savedOAuthGateways[id] || gatewayRequests.current.has(id)) return; + const controller = new AbortController(); + gatewayRequests.current.set(id, controller); + void api + .get(`/v1/mcp-servers/${encodeURIComponent(id)}`, undefined, controller.signal) + .then((gateway) => { + if (!controller.signal.aborted) { + setSavedOAuthGateways((previous) => + previous[id] ? previous : { ...previous, [id]: gateway }, + ); + } + }) + .catch(() => undefined) + .finally(() => { + if (gatewayRequests.current.get(id) === controller) { + gatewayRequests.current.delete(id); + } + }); + }); + }, [oauthGatewayIdsKey, canReadOAuthStatuses, savedOAuthGateways, abortGatewayRequests]); + const validationTargets = Object.fromEntries( + oauthGatewayIds.map((id) => [ + id, + savedOAuthGateways[id] ? oauthConfigurationKey(savedOAuthGateways[id]) : id, + ]), + ); + const { + entries: validationEntries, + test: testOAuthConnection, + reset: resetValidation, + } = useOAuthConnectionValidation(canReadOAuthStatuses && canUpdateServer, validationTargets); + const canTestOAuthServer = useCallback( + (server: CatalogServer) => { + const gateway = server.gateway_id ? savedOAuthGateways[server.gateway_id] : undefined; + return ( + canReadOAuthStatuses && + canUpdateServer && + !!gateway && + canValidateOAuthConnection(gateway, oauthStatuses[gateway.id]) + ); + }, + [canReadOAuthStatuses, canUpdateServer, savedOAuthGateways, oauthStatuses], + ); + const getTestAction = useCallback( + (server: CatalogServer) => + catalogTestAction(server, { + status: oauthStatuses[server.gateway_id ?? ""], + canTest: canReadOAuthStatuses, + canUpdate: canUpdateServer, + canValidate: canTestOAuthServer(server), + pending: isAdding(server.id) || isTesting(server.id) || isDisconnecting(server.id), + }), + [ + oauthStatuses, + canReadOAuthStatuses, + canUpdateServer, + canTestOAuthServer, + isAdding, + isTesting, + isDisconnecting, + ], + ); const canDisconnect = !permissionsLoading && hasPermission("gateways.delete"); const { filters, updateQuery, toggleFilterOption, clearFilterSection, clearAllFilters } = useCatalogFilters(); @@ -523,12 +609,18 @@ export function ServerCatalog() { const refreshGatewayData = useCallback( async (gatewayId: string, server: Pick) => { + const gatewayRefresh = api + .get(`/v1/mcp-servers/${encodeURIComponent(gatewayId)}`) + .then((gateway) => + setSavedOAuthGateways((previous) => ({ ...previous, [gatewayId]: gateway })), + ); const componentRefreshPromise = canUpdateServer ? serversApi.fetchToolsAfterOAuth(gatewayId) : Promise.resolve(undefined); const [, componentRefresh] = await Promise.allSettled([ refreshOAuthStatus(gatewayId), componentRefreshPromise, + gatewayRefresh, ]); if (componentRefresh.status === "rejected") { showRegistrationNotification({ @@ -746,7 +838,7 @@ export function ServerCatalog() { setOAuthFormNotification({ id: `oauth:${oauthServer.id}`, type: "info", - message: oauthErrorMessage(intl, error), + message: catalogOAuthErrorMessage(intl, error), }); return false; } @@ -771,6 +863,7 @@ export function ServerCatalog() { setOAuthAuthorizing(true); setOAuthFormNotification(undefined); try { + resetValidation(gatewayId); await serversApi.triggerOAuthAuthorization(gatewayId, authWindow); if (canUpdateServer) await serversApi.toggleEnabled(gatewayId, true); await refreshGatewayData(gatewayId, oauthServer); @@ -787,7 +880,7 @@ export function ServerCatalog() { setOAuthFormNotification({ id: `oauth:${oauthServer.id}`, type: "info", - message: oauthErrorMessage(intl, error), + message: catalogOAuthErrorMessage(intl, error), }); return false; } finally { @@ -799,6 +892,7 @@ export function ServerCatalog() { canUpdateServer, oauthServer, pendingOAuthGatewayId, + resetValidation, refreshGatewayData, refreshCatalogSilently, registerServer, @@ -811,6 +905,7 @@ export function ServerCatalog() { if (!server.gateway_id || !beginAdding(server.id)) return; dismissRegistrationNotification(`oauth:${server.id}`); try { + resetValidation(server.gateway_id); await serversApi.triggerOAuthAuthorization(server.gateway_id); if (canUpdateServer) await serversApi.toggleEnabled(server.gateway_id, true); await refreshGatewayData(server.gateway_id, server); @@ -825,7 +920,7 @@ export function ServerCatalog() { showRegistrationNotification({ id: `oauth:${server.id}`, type: "info", - message: oauthErrorMessage(intl, error), + message: catalogOAuthErrorMessage(intl, error), }); } finally { endAdding(server.id); @@ -833,6 +928,7 @@ export function ServerCatalog() { }, [ beginAdding, + resetValidation, canUpdateServer, dismissRegistrationNotification, endAdding, @@ -846,17 +942,30 @@ export function ServerCatalog() { const handleTest = useCallback( async (server: CatalogServer) => { - if ( - server.requires_oauth_config || - isAdding(server.id) || - isDisconnecting(server.id) || - !beginTesting(server.id) - ) { + const action = getTestAction(server); + if (action.availability !== "enabled" || !beginTesting(server.id)) { return; } dismissRegistrationNotification(`test:${server.id}`); + if (action.kind === "oauth") { + const id = server.gateway_id!; + await testOAuthConnection(id, async (signal) => { + const gateway = await api.get( + `/v1/mcp-servers/${encodeURIComponent(id)}`, + undefined, + signal, + ); + if (!canValidateOAuthConnection(gateway, oauthStatuses[id])) { + throw new ApiError(422, null, "Gateway is not eligible for OAuth validation"); + } + return { gatewayId: id, baseUrl: gateway.url }; + }); + endTesting(server.id); + return; + } + try { const result = await testCatalogServer(server.url); if (!result) { @@ -906,13 +1015,14 @@ export function ServerCatalog() { } }, [ + getTestAction, beginTesting, dismissRegistrationNotification, + testOAuthConnection, + oauthStatuses, endTesting, - intl, - isAdding, - isDisconnecting, showRegistrationNotification, + intl, ], ); @@ -1065,6 +1175,7 @@ export function ServerCatalog() { let pollController: AbortController | null = null; try { + resetValidation(gatewayId); const response = await disconnectCatalogGateway(gatewayId); if (response.status === 202) { setDisconnectServer(null); @@ -1138,6 +1249,7 @@ export function ServerCatalog() { } }, [ beginDisconnecting, + resetValidation, disconnectServer, endDisconnecting, intl, @@ -1297,12 +1409,32 @@ export function ServerCatalog() { onView={handleView} onAdd={handleAdd} addingServerIds={addingServerIds} - onTest={(server) => void handleTest(server)} + onTest={handleTest} + getTestAction={getTestAction} + renderOAuthValidation={(server) => { + if ( + !server.gateway_id || + !canTestOAuthServer(server) || + isAdding(server.id) || + isDisconnecting(server.id) + ) { + return null; + } + const id = server.gateway_id; + return ( + void handleTest(server)} + onCancel={() => resetValidation(id)} + onAuthorize={() => void handleAuthorize(server)} + /> + ); + }} onAuthorize={(server) => void handleAuthorize(server)} onDisconnect={handleDisconnect} testingServerIds={testingServerIds} disconnectingServerIds={disconnectingServerIds} - canTest={canTest} canDisconnect={canDisconnect} oauthStatuses={oauthStatuses} addErrors={addErrors} diff --git a/src/pages/Servers.tsx b/src/pages/Servers.tsx index 26394ea6..50cdd023 100644 --- a/src/pages/Servers.tsx +++ b/src/pages/Servers.tsx @@ -26,7 +26,7 @@ import { OAuthConnectionValidationPanel } from "@/components/servers/OAuthConnec import { canValidateOAuthConnection, oauthConfigurationKey, - oauthErrorMessage, + oauthAuthorizationErrorMessage, } from "@/lib/oauthConnectionValidation"; import { isOAuthServer } from "@/lib/serverStatus"; @@ -327,14 +327,7 @@ export function Servers() { if (!(err instanceof OAuthCancelledError)) setOAuthDialogServerId(id); let reason: string; if (err instanceof OAuthAuthorizationError) { - const guidance = oauthErrorMessage(intl, err.errorCode); - reason = - err.description && err.description !== guidance - ? intl.formatMessage( - { id: "mcpServer.oauthValidation.providerError" }, - { guidance, description: err.description }, - ) - : guidance; + reason = oauthAuthorizationErrorMessage(intl, err); } else { reason = err instanceof OAuthCancelledError ? "" : sanitizeError(err); }