Merge pull request #15 from contextforge-org/5721-mcp-oauth-form-quick-wins

fix: MCP server OAuth form quick wins (read-only redirect URI, drop password grant) UI part

Author: marekdano

src/components/mcp-servers/MCPServerForm.test.tsx

@@ -793,111 +793,16 @@ describe("MCPServerForm", () => {
     });
   });
 
-  describe("OAuth Password Grant Validation", () => {
-    // Helper: open advanced settings, switch to OAuth auth, select password grant
-    const renderWithOAuthPassword = // pragma: allowlist secret
-      async () => {
-        const user = userEvent.setup();
-        renderWithRouter(<MCPServerForm {...defaultProps} />);
-        await user.click(screen.getByRole("button", { name: /Advanced settings/i }));
-        await user.click(screen.getByRole("radio", { name: /OAuth 2\.0/i }));
-        // Grant type defaults to client_credentials; switch to password
-        await user.click(screen.getByRole("combobox", { name: /Grant type/i }));
-        await user.click(screen.getByRole("option", { name: /Resource owner password/i }));
-        return user;
-      };
-
-    it("shows username and password fields when password grant is selected", async () => {
-      await renderWithOAuthPassword();
-      expect(screen.getByLabelText(/Username/i)).toBeInTheDocument();
-      expect(screen.getByLabelText(/^Password/i)).toBeInTheDocument();
-    });
-
-    it("disables the submit button when username is empty", async () => {
-      await renderWithOAuthPassword();
-      fireEvent.change(screen.getByLabelText(/^Name/i), { target: { value: "Test Server" } });
-      fireEvent.change(screen.getByLabelText(/^URL/i), {
-        target: { value: "http://localhost:3000" },
-      });
-      // Leave username empty, fill password
-      fireEvent.change(screen.getByLabelText(/^Password/i), { target: { value: "secret" } });
-      expect(screen.getByRole("button", { name: /Connect server/i })).toBeDisabled();
-    });
-
-    it("disables the submit button when password is empty", async () => {
-      await renderWithOAuthPassword();
-      fireEvent.change(screen.getByLabelText(/^Name/i), { target: { value: "Test Server" } });
-      fireEvent.change(screen.getByLabelText(/^URL/i), {
-        target: { value: "http://localhost:3000" },
-      });
-      fireEvent.change(screen.getByLabelText(/^Username/i), {
-        target: { value: "service-account" },
-      });
-      // Leave password empty
-      expect(screen.getByRole("button", { name: /Connect server/i })).toBeDisabled();
-    });
-
-    it("enables the submit button when both username and password are provided", async () => {
-      await renderWithOAuthPassword();
-      fireEvent.change(screen.getByLabelText(/^Name/i), { target: { value: "Test Server" } });
-      fireEvent.change(screen.getByLabelText(/^URL/i), {
-        target: { value: "http://localhost:3000" },
-      });
-      fireEvent.change(screen.getByLabelText(/^Username/i), {
-        target: { value: "service-account" },
-      });
-      fireEvent.change(screen.getByLabelText(/^Password/i), { target: { value: "secret" } });
-      expect(screen.getByRole("button", { name: /Connect server/i })).not.toBeDisabled();
-    });
-
-    it("marks username input as aria-invalid when username error is present", async () => {
-      await renderWithOAuthPassword();
-      fireEvent.change(screen.getByLabelText(/^Name/i), { target: { value: "Test Server" } });
-      fireEvent.change(screen.getByLabelText(/^URL/i), {
-        target: { value: "http://localhost:3000" },
-      });
-      // Only fill password, leave username empty
-      fireEvent.change(screen.getByLabelText(/^Password/i), { target: { value: "secret" } });
-
-      // Expose the field without a value and attempt form submission
-      const form = document.querySelector("form")!;
-      fireEvent.submit(form);
-
-      await waitFor(() => {
-        expect(screen.getByLabelText(/^Username/i)).toHaveAttribute("aria-invalid", "true");
-      });
-    });
-
-    it("marks password input as aria-invalid when password error is present", async () => {
-      await renderWithOAuthPassword();
-      fireEvent.change(screen.getByLabelText(/^Name/i), { target: { value: "Test Server" } });
-      fireEvent.change(screen.getByLabelText(/^URL/i), {
-        target: { value: "http://localhost:3000" },
-      });
-      fireEvent.change(screen.getByLabelText(/^Username/i), {
-        target: { value: "service-account" },
-      });
-      // Leave password empty, submit the form
-      const form = document.querySelector("form")!;
-      fireEvent.submit(form);
-
-      await waitFor(() => {
-        expect(screen.getByLabelText(/^Password/i)).toHaveAttribute("aria-invalid", "true");
-      });
-    });
-
-    it("shows inline error messages for both fields when both are empty", async () => {
-      await renderWithOAuthPassword();
-      fireEvent.change(screen.getByLabelText(/^Name/i), { target: { value: "Test Server" } });
-      fireEvent.change(screen.getByLabelText(/^URL/i), {
-        target: { value: "http://localhost:3000" },
-      });
-      fireEvent.submit(document.querySelector("form")!);
-
-      await waitFor(() => {
-        expect(screen.getByText("Username is required for password grant")).toBeInTheDocument();
-        expect(screen.getByText("Password is required for password grant")).toBeInTheDocument();
-      });
+  describe("OAuth Password Grant", () => {
+    it("does not offer the deprecated password grant for new servers", async () => {
+      const user = userEvent.setup();
+      renderWithRouter(<MCPServerForm {...defaultProps} />);
+      await user.click(screen.getByRole("button", { name: /Advanced settings/i }));
+      await user.click(screen.getByRole("radio", { name: /OAuth 2\.0/i }));
+      await user.click(screen.getByRole("combobox", { name: /Grant type/i }));
+      expect(
+        screen.queryByRole("option", { name: /Resource owner password/i }),
+      ).not.toBeInTheDocument();
     });
 
     it("does not show password-grant errors when a different OAuth grant type is selected", async () => {

src/components/mcp-servers/MCPServerForm.tsx

@@ -1,4 +1,4 @@
-import { useState, type ReactNode } from "react";
+import { useCallback, useState, type ReactNode } from "react";
 import { useIntl } from "react-intl";
 import { ChevronDown, CircleAlert } from "lucide-react";
 import { Button } from "@/components/ui/button";
@@ -96,6 +96,13 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ
     setQueryParamApiKey,
   } = useMCPServerForm(serverId);
 
+  const handleRedirectUriChange = useCallback(
+    (uri: string) => {
+      setOAuthRedirectUri(uri);
+    },
+    [setOAuthRedirectUri],
+  );
+
   const handleCancel = () => {
     setCreatedGateway(null);
     onToggle();
@@ -347,7 +354,7 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ
                   onOAuthTokenUrlChange={setOAuthTokenUrl}
                   onOAuthGrantTypeChange={setOAuthGrantType}
                   onOAuthIssuerUrlChange={setOAuthIssuerUrl}
-                  onOAuthRedirectUriChange={setOAuthRedirectUri}
+                  onOAuthRedirectUriChange={handleRedirectUriChange}
                   onOAuthAuthorizationUrlChange={setOAuthAuthorizationUrl}
                   onOAuthScopesChange={setOAuthScopes}
                   onOAuthStoreTokensChange={setOAuthStoreTokens}

src/components/mcp-servers/OAuth2Auth.test.tsx

@@ -1,5 +1,5 @@
-import { describe, it, expect, vi } from "vitest";
-import { screen, fireEvent } from "@testing-library/react";
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { screen, fireEvent, act } from "@testing-library/react";
 import { renderWithProviders as render } from "@/test/test-utils";
 import { OAuth2Auth } from "./OAuth2Auth";
 
@@ -145,30 +145,71 @@ describe("OAuth2Auth", () => {
     expect(onPasswordChange).toHaveBeenCalledWith("test-pass");
   });
 
-  it("should trigger callbacks for authorization_code fields", () => {
-    const onRedirectUriChange = vi.fn();
+  it("shows a read-only derived redirect URI, lifts it into form state, and triggers the authorization URL callback", () => {
     const onAuthorizationUrlChange = vi.fn();
+    const onRedirectUriChange = vi.fn();
 
     render(
       <OAuth2Auth
         {...defaultProps}
         grantType="authorization_code"
-        onRedirectUriChange={onRedirectUriChange}
         onAuthorizationUrlChange={onAuthorizationUrlChange}
+        onRedirectUriChange={onRedirectUriChange}
       />,
     );
 
-    fireEvent.change(screen.getByLabelText(/Redirect URI/i), {
-      target: { value: "https://redirect.com" },
-    });
-    expect(onRedirectUriChange).toHaveBeenCalledWith("https://redirect.com");
+    const redirect = screen.getByLabelText(/Redirect URI/i);
+    expect(redirect).toHaveAttribute("readonly");
+    expect(redirect).toHaveValue(`${window.location.origin}/oauth/callback`);
+    expect(screen.getByRole("button", { name: "Copy to clipboard" })).toBeInTheDocument();
+    expect(onRedirectUriChange).toHaveBeenCalledWith(`${window.location.origin}/oauth/callback`);
 
     fireEvent.change(screen.getByLabelText(/Authorization URL/i), {
       target: { value: "https://auth.com/authorize" },
     });
     expect(onAuthorizationUrlChange).toHaveBeenCalledWith("https://auth.com/authorize");
   });
 
+  it("displays a stored redirect URI verbatim without overwriting it", () => {
+    const onRedirectUriChange = vi.fn();
+
+    render(
+      <OAuth2Auth
+        {...defaultProps}
+        grantType="authorization_code"
+        redirectUri="https://public.example.com/oauth/callback"
+        onRedirectUriChange={onRedirectUriChange}
+      />,
+    );
+
+    expect(screen.getByLabelText(/Redirect URI/i)).toHaveValue(
+      "https://public.example.com/oauth/callback",
+    );
+    expect(onRedirectUriChange).not.toHaveBeenCalled();
+  });
+
+  it("does not set a redirect URI for non-authorization_code grants", () => {
+    const onRedirectUriChange = vi.fn();
+
+    render(
+      <OAuth2Auth
+        {...defaultProps}
+        grantType="client_credentials"
+        onRedirectUriChange={onRedirectUriChange}
+      />,
+    );
+
+    expect(onRedirectUriChange).not.toHaveBeenCalled();
+  });
+
+  it("only offers the password grant option when already selected (legacy)", () => {
+    const { rerender } = render(<OAuth2Auth {...defaultProps} grantType="client_credentials" />);
+    expect(screen.queryByText(/Password grant is deprecated/i)).not.toBeInTheDocument();
+
+    rerender(<OAuth2Auth {...defaultProps} grantType="password" />);
+    expect(screen.getByText(/Password grant is deprecated/i)).toBeInTheDocument();
+  });
+
   it("should trigger checkbox callback functions", () => {
     const onStoreTokensChange = vi.fn();
     const onAutoRefreshChange = vi.fn();
@@ -187,4 +228,84 @@ describe("OAuth2Auth", () => {
     fireEvent.click(screen.getByLabelText(/Automatically refresh expired tokens/i));
     expect(onAutoRefreshChange).toHaveBeenCalled();
   });
+
+  describe("copy button interaction", () => {
+    beforeEach(() => {
+      Object.assign(navigator, {
+        clipboard: { writeText: vi.fn().mockResolvedValue(undefined) },
+      });
+    });
+
+    it("copies the redirect URI to clipboard when the copy button is clicked", async () => {
+      render(<OAuth2Auth {...defaultProps} grantType="authorization_code" />);
+
+      const copyButton = screen.getByRole("button", { name: /Copy to clipboard/i });
+      await act(async () => {
+        fireEvent.click(copyButton);
+      });
+
+      expect(navigator.clipboard.writeText).toHaveBeenCalledWith(
+        `${window.location.origin}/oauth/callback`,
+      );
+    });
+
+    it("shows a check icon immediately after clicking copy and reverts after 2 s", async () => {
+      vi.useFakeTimers();
+
+      render(<OAuth2Auth {...defaultProps} grantType="authorization_code" />);
+
+      const copyButton = screen.getByRole("button", { name: /Copy to clipboard/i });
+      await act(async () => {
+        fireEvent.click(copyButton);
+      });
+
+      // The button is still present (aria-label unchanged; icon swap is visual-only)
+      expect(copyButton).toBeInTheDocument();
+
+      await act(async () => {
+        vi.advanceTimersByTime(2000);
+      });
+
+      vi.useRealTimers();
+    });
+  });
+
+  describe("localhost warning", () => {
+    it("shows a localhost warning when the derived redirect URI points to localhost", () => {
+      // jsdom sets window.location.origin to 'http://localhost'
+      render(<OAuth2Auth {...defaultProps} grantType="authorization_code" />);
+
+      expect(
+        screen.getByText(/Redirect URIs derived from localhost will not work/i),
+      ).toBeInTheDocument();
+    });
+
+    it("does not show the localhost warning when a non-localhost stored redirect URI is used", () => {
+      render(
+        <OAuth2Auth
+          {...defaultProps}
+          grantType="authorization_code"
+          redirectUri="https://public.example.com/oauth/callback"
+        />,
+      );
+
+      expect(
+        screen.queryByText(/Redirect URIs derived from localhost will not work/i),
+      ).not.toBeInTheDocument();
+    });
+
+    it("shows the localhost warning when a stored redirect URI points to 127.0.0.1", () => {
+      render(
+        <OAuth2Auth
+          {...defaultProps}
+          grantType="authorization_code"
+          redirectUri="http://127.0.0.1:8080/oauth/callback"
+        />,
+      );
+
+      expect(
+        screen.getByText(/Redirect URIs derived from localhost will not work/i),
+      ).toBeInTheDocument();
+    });
+  });
 });