fix(mcp): state the keep-existing convention for blank client fields and add explicit app removal on edit

This commit is contained in:
Tin 2026-07-10 09:51:10 -07:00
parent 57051d36d6
commit cf1b407fbe
3 changed files with 77 additions and 4 deletions

View file

@ -1,5 +1,5 @@
import React from "react";
import { Button, Form, Input } from "antd";
import { Button, Checkbox, Form, Input } from "antd";
import { isClientForwardedTokenMode } from "./types";
interface PassthroughOAuthFlow {
@ -19,13 +19,26 @@ interface PassthroughOAuthFlow {
* pre-registered Slack app); unlike the token they ARE saved onto the server
* as declared config, so internal users' Authorize relays through the org's
* app instead of dead-ending on upstreams that cannot mint clients.
*
* Blank fields follow the same convention as the M2M credential fields: on
* create they mean "no app configured" (dynamic client registration), while on
* edit the backend's partial update keeps whatever app is already stored, so
* blanks mean "keep existing". Removing a stored app is therefore an explicit
* action (the checkbox below, edit only), which saves an explicit-null
* credential write instead of omitting the field.
*/
export default function PassthroughAuthorizeSection({
authType,
oauthFlow,
isEditing = false,
removeStoredApp = false,
onRemoveStoredAppChange,
}: {
authType?: string | null;
oauthFlow: PassthroughOAuthFlow;
isEditing?: boolean;
removeStoredApp?: boolean;
onRemoveStoredAppChange?: (remove: boolean) => void;
}) {
if (!isClientForwardedTokenMode(authType)) return null;
const authorizeButtonLabels: Record<string, string> = {
@ -33,6 +46,9 @@ export default function PassthroughAuthorizeSection({
exchanging: "Exchanging authorization code...",
};
const authorizeButtonLabel = authorizeButtonLabels[oauthFlow.status] ?? "Authorize & Fetch Tools (browser-only)";
const blankMeaning = isEditing
? "Leave blank to keep the currently saved app (if any)"
: "Leave blank to use dynamic client registration";
return (
<div className="rounded-lg border border-dashed border-gray-300 p-4 space-y-2 mb-4">
<p className="text-sm text-gray-600">
@ -47,7 +63,8 @@ export default function PassthroughAuthorizeSection({
extra="Set this to make everyone authorize through a specific app; required for upstreams without dynamic client registration (e.g. a pre-registered Slack app)."
>
<Input.Password
placeholder="Leave blank to use dynamic client registration"
placeholder={blankMeaning}
disabled={removeStoredApp}
className="rounded-lg border-gray-300 focus:border-blue-500 focus:ring-blue-500"
/>
</Form.Item>
@ -57,9 +74,17 @@ export default function PassthroughAuthorizeSection({
>
<Input.Password
placeholder="Leave blank for public clients / PKCE"
disabled={removeStoredApp}
className="rounded-lg border-gray-300 focus:border-blue-500 focus:ring-blue-500"
/>
</Form.Item>
{isEditing && onRemoveStoredAppChange && (
<Checkbox checked={removeStoredApp} onChange={(e) => onRemoveStoredAppChange(e.target.checked)}>
<span className="text-sm text-gray-700">
Remove the saved OAuth app on save (the server goes back to dynamic client registration)
</span>
</Checkbox>
)}
<Button
onClick={oauthFlow.startOAuthFlow}
disabled={oauthFlow.status === "authorizing" || oauthFlow.status === "exchanging"}

View file

@ -1400,7 +1400,7 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => {
const user = userEvent.setup({ delay: null });
await user.type(
screen.getByPlaceholderText("Leave blank to use dynamic client registration"),
screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)"),
"org-app-client-id",
);
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "org-app-secret");
@ -1444,7 +1444,7 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => {
const user = userEvent.setup({ delay: null });
await user.type(
screen.getByPlaceholderText("Leave blank to use dynamic client registration"),
screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)"),
"org-app-client-id",
);
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "org-app-secret");
@ -1477,6 +1477,42 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => {
},
);
it("sends an explicit-null credential write when removing the saved app for true_passthrough", async () => {
vi.mocked(networking.updateMCPServer).mockResolvedValue({
...interactiveOAuthServer,
auth_type: "true_passthrough",
});
render(
<MCPServerEdit
mcpServer={{ ...interactiveOAuthServer, auth_type: "true_passthrough" }}
accessToken="access-token"
userID="user-1"
onCancel={vi.fn()}
onSuccess={vi.fn()}
availableAccessGroups={[]}
/>,
);
// Blank fields keep the stored app (the backend merges partial credential updates), so the
// edit form states that convention and removal is an explicit checkbox that saves nulls.
expect(screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)")).toBeInTheDocument();
fireEvent.click(
screen.getByRole("checkbox", {
name: /Remove the saved OAuth app on save/,
}),
);
await act(async () => {
fireEvent.click(screen.getAllByRole("button", { name: "Save Changes" })[0]);
});
await waitFor(() => expect(networking.updateMCPServer).toHaveBeenCalledTimes(1));
const [, payload] = vi.mocked(networking.updateMCPServer).mock.calls[0];
expect(payload.credentials).toEqual({ client_id: null, client_secret: null });
});
it("forwards a newly authorized browser-held token for tool loading before the form is saved", async () => {
// Regression: fetchTools keyed the browser-held decision off the saved mcpServer.auth_type, so
// after switching the form to true_passthrough and authorizing, the fresh token was not sent as

View file

@ -77,6 +77,7 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
const [toolsError, setToolsError] = useState<string | null>(null);
const [searchValue, setSearchValue] = useState<string>("");
const [aliasManuallyEdited, setAliasManuallyEdited] = useState(false);
const [removeStoredApp, setRemoveStoredApp] = useState(false);
const [allowedTools, setAllowedTools] = useState<string[]>([]);
const [hasToolAllowlistInteraction, setHasToolAllowlistInteraction] = useState(false);
const [toolNameToDisplayName, setToolNameToDisplayName] = useState<Record<string, string>>({});
@ -865,6 +866,14 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
payload.credentials = credentialsPayload;
}
// Explicit removal of a saved app for the client-forwarded modes. Blank fields are the
// keep-existing convention (the backend merges partial credential updates), so removal must be
// an explicit-null write: encrypt skips nulls and the merge overrides the stored keys, which
// returns the server to dynamic client registration.
if (removeStoredApp && isClientForwardedTokenMode(restValues.auth_type)) {
payload.credentials = { client_id: null, client_secret: null };
}
const updated = await updateMCPServer(accessToken, payload);
// Persist the token staged via "Authorize & Fetch" (mirrors the create flow's
@ -1051,6 +1060,9 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
error: oauthError,
tokenResponse: oauthTokenResponse,
}}
isEditing
removeStoredApp={removeStoredApp}
onRemoveStoredAppChange={setRemoveStoredApp}
/>
</>
)}