feat(oauth): make the refresh_token grant opt-out per server
The Inspector always declared `refresh_token` in its OAuth client metadata. The SDK's `determineScope()` then appends `offline_access` whenever the authorization server advertises it and the client declares that grant, and `startAuthorization()` appends `prompt=consent` whenever `offline_access` is in scope. Against Microsoft Entra ID that forced consent prompt routes a non-admin user into the admin-consent workflow and fails with AADSTS90094 — even after a tenant admin has consented. Adds a per-server "Request refresh token" setting (default on) that drops the grant from the client metadata, breaking that chain at its source. Persisted as `oauth.requestRefreshToken` and written only when off, so entries that never touch it keep a byte-stable round-trip. Web edits it in Server Settings → Authorization; the CLI and TUI pick it up from the same per-server settings via `buildRunnerClientAuthOptions`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
9d0448b644
commit
e2fb78d999
9 changed files with 405 additions and 5 deletions
|
|
@ -2602,7 +2602,8 @@ function App() {
|
|||
savedSettings.oauthScopes ||
|
||||
serverAuthorizationParams ||
|
||||
serverEndpointOverrides ||
|
||||
savedSettings.enterpriseManaged)
|
||||
savedSettings.enterpriseManaged ||
|
||||
savedSettings.oauthRequestRefreshToken === false)
|
||||
? {
|
||||
...(savedSettings.oauthClientId && {
|
||||
clientId: savedSettings.oauthClientId,
|
||||
|
|
@ -2620,6 +2621,11 @@ function App() {
|
|||
...(savedSettings.enterpriseManaged && {
|
||||
enterpriseManaged: true,
|
||||
}),
|
||||
// #2068: only the explicit opt-out is forwarded; omitting the key
|
||||
// leaves the provider's default (declare `refresh_token`) in place.
|
||||
...(savedSettings.oauthRequestRefreshToken === false && {
|
||||
requestRefreshToken: false,
|
||||
}),
|
||||
}
|
||||
: undefined;
|
||||
const oauth =
|
||||
|
|
|
|||
|
|
@ -706,9 +706,51 @@ describe("ServerSettingsForm", () => {
|
|||
authorizationUrl: "",
|
||||
tokenUrl: "",
|
||||
enterpriseManaged: false,
|
||||
requestRefreshToken: true,
|
||||
});
|
||||
});
|
||||
|
||||
// #2068 — the refresh-token opt-out. On by default; unchecking it is what
|
||||
// stops the SDK adding `offline_access` and then `prompt=consent`.
|
||||
it("renders Request refresh token checked by default", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={emptySettings}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByLabelText("Request refresh token")).toBeChecked();
|
||||
});
|
||||
|
||||
it("renders Request refresh token unchecked when the server opted out", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{ ...emptySettings, oauthRequestRefreshToken: false }}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByLabelText("Request refresh token")).not.toBeChecked();
|
||||
});
|
||||
|
||||
it("toggles Request refresh token through onOAuthChange", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onOAuthChange = vi.fn();
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
onOAuthChange={onOAuthChange}
|
||||
settings={emptySettings}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
await user.click(screen.getByLabelText("Request refresh token"));
|
||||
expect(onOAuthChange).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({ requestRefreshToken: false }),
|
||||
);
|
||||
});
|
||||
|
||||
// #2018 — custom authorization-request parameters.
|
||||
it("shows the empty hint for authorization parameters when none are set", () => {
|
||||
renderWithMantine(
|
||||
|
|
@ -1112,6 +1154,7 @@ describe("ServerSettingsForm", () => {
|
|||
authorizationUrl: "",
|
||||
tokenUrl: "",
|
||||
enterpriseManaged: true,
|
||||
requestRefreshToken: true,
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -1219,6 +1262,7 @@ describe("ServerSettingsForm", () => {
|
|||
authorizationUrl: "",
|
||||
tokenUrl: "",
|
||||
enterpriseManaged: false,
|
||||
requestRefreshToken: true,
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -518,6 +518,8 @@ export function ServerSettingsForm({
|
|||
tokenUrl: settings.oauthTokenUrl ?? "",
|
||||
enterpriseManaged: settings.enterpriseManaged ?? false,
|
||||
onInsufficientScope: settings.oauthOnInsufficientScope,
|
||||
// Unset means the default, on — only an explicit opt-out is stored.
|
||||
requestRefreshToken: settings.oauthRequestRefreshToken ?? true,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -879,6 +881,17 @@ export function ServerSettingsForm({
|
|||
) : null
|
||||
}
|
||||
/>
|
||||
<Checkbox
|
||||
label="Request refresh token"
|
||||
description="When checked, the Inspector registers with the refresh_token grant so it can renew access tokens without a new sign-in. Uncheck it for authorization servers where that grant forces an unwanted consent prompt — the SDK adds offline_access to the requested scope whenever the grant is declared, and then prompt=consent to the authorization request. On Microsoft Entra ID that routes a non-admin user into the admin-consent workflow (AADSTS90094) even after a tenant admin has consented."
|
||||
checked={settings.oauthRequestRefreshToken ?? true}
|
||||
onChange={(e) =>
|
||||
onOAuthChange({
|
||||
...currentOAuth(),
|
||||
requestRefreshToken: e.currentTarget.checked,
|
||||
})
|
||||
}
|
||||
/>
|
||||
<Stack gap="xs">
|
||||
<FieldLabel>Additional authorization parameters</FieldLabel>
|
||||
<FieldDescription>
|
||||
|
|
|
|||
|
|
@ -119,6 +119,43 @@ describe("ServerSettingsModal", () => {
|
|||
);
|
||||
});
|
||||
|
||||
// #2068 — only the opt-out is persisted; re-checking the box clears the field
|
||||
// rather than writing `true`, so a default-on server keeps a minimal entry.
|
||||
it("maps the refresh-token opt-out into settings, and back to unset", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onSettingsChange = vi.fn();
|
||||
const { rerender } = renderWithMantine(
|
||||
<ServerSettingsModal
|
||||
opened
|
||||
settings={initialSettings}
|
||||
serverType="streamable-http"
|
||||
isStdio={false}
|
||||
onClose={vi.fn()}
|
||||
onSettingsChange={onSettingsChange}
|
||||
/>,
|
||||
);
|
||||
await user.click(screen.getByRole("button", { name: /OAuth Settings/i }));
|
||||
await user.click(screen.getByLabelText("Request refresh token"));
|
||||
expect(onSettingsChange).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({ oauthRequestRefreshToken: false }),
|
||||
);
|
||||
|
||||
rerender(
|
||||
<ServerSettingsModal
|
||||
opened
|
||||
settings={{ ...initialSettings, oauthRequestRefreshToken: false }}
|
||||
serverType="streamable-http"
|
||||
isStdio={false}
|
||||
onClose={vi.fn()}
|
||||
onSettingsChange={onSettingsChange}
|
||||
/>,
|
||||
);
|
||||
await user.click(screen.getByLabelText("Request refresh token"));
|
||||
expect(onSettingsChange).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({ oauthRequestRefreshToken: undefined }),
|
||||
);
|
||||
});
|
||||
|
||||
it("maps the selected protocol era into settings (#1626)", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onSettingsChange = vi.fn();
|
||||
|
|
|
|||
|
|
@ -187,6 +187,10 @@ export function ServerSettingsModal({
|
|||
// the SDK's `reauthorize` behavior without writing a spurious field.
|
||||
oauthOnInsufficientScope:
|
||||
oauth.onInsufficientScope === "throw" ? "throw" : undefined,
|
||||
// #2068: persist only the non-default (off). `undefined` means on, so a
|
||||
// server that never touched the switch writes no field at all.
|
||||
oauthRequestRefreshToken:
|
||||
oauth.requestRefreshToken === false ? false : undefined,
|
||||
});
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -464,6 +464,48 @@ describe("OAuthNavigation", () => {
|
|||
expect(provider.clientMetadata.application_type).toBe("native");
|
||||
});
|
||||
|
||||
describe("requestRefreshToken (#2068)", () => {
|
||||
function makeProviderWithRefresh(
|
||||
requestRefreshToken: boolean | undefined,
|
||||
): BaseOAuthClientProvider {
|
||||
return new BaseOAuthClientProvider(SERVER, {
|
||||
storage: makeStorage(),
|
||||
redirectUrlProvider: new MutableRedirectUrlProvider(),
|
||||
navigation: new CallbackNavigation(vi.fn()),
|
||||
requestRefreshToken,
|
||||
});
|
||||
}
|
||||
|
||||
it("declares the refresh_token grant by default", () => {
|
||||
expect(makeProvider(makeStorage()).clientMetadata.grant_types).toEqual([
|
||||
"authorization_code",
|
||||
"refresh_token",
|
||||
]);
|
||||
});
|
||||
|
||||
it("declares the refresh_token grant when explicitly enabled", () => {
|
||||
expect(
|
||||
makeProviderWithRefresh(true).clientMetadata.grant_types,
|
||||
).toEqual(["authorization_code", "refresh_token"]);
|
||||
});
|
||||
|
||||
it("treats an omitted setting as enabled", () => {
|
||||
expect(
|
||||
makeProviderWithRefresh(undefined).clientMetadata.grant_types,
|
||||
).toEqual(["authorization_code", "refresh_token"]);
|
||||
});
|
||||
|
||||
// The point of the opt-out: the SDK's determineScope() only appends
|
||||
// `offline_access` when the client metadata declares `refresh_token`, and
|
||||
// startAuthorization() only forces `prompt=consent` when `offline_access`
|
||||
// is in scope. Dropping the grant is what breaks that chain.
|
||||
it("drops the refresh_token grant when disabled", () => {
|
||||
expect(
|
||||
makeProviderWithRefresh(false).clientMetadata.grant_types,
|
||||
).toEqual(["authorization_code"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("SEP-2352 issuer threading", () => {
|
||||
it("forwards ctx.issuer to storage on clientInformation/tokens reads", async () => {
|
||||
const storage = makeStorage();
|
||||
|
|
|
|||
|
|
@ -86,6 +86,34 @@ describe("runner client auth options", () => {
|
|||
expect(opts.oauth?.clientId).toBe("resource-client");
|
||||
});
|
||||
|
||||
// #2068 — the refresh-token opt-out is a per-server setting, so the CLI/TUI
|
||||
// leg honors it exactly as the web leg does.
|
||||
it("buildRunnerClientAuthOptions forwards the refresh-token opt-out", () => {
|
||||
const base: InspectorServerSettings = {
|
||||
requestTimeout: 0,
|
||||
connectionTimeout: 0,
|
||||
taskTtl: 60000,
|
||||
maxFetchRequests: 10,
|
||||
metadata: {},
|
||||
headers: [],
|
||||
env: [],
|
||||
roots: [],
|
||||
};
|
||||
expect(
|
||||
buildRunnerClientAuthOptions(
|
||||
{},
|
||||
{
|
||||
...base,
|
||||
oauthRequestRefreshToken: false,
|
||||
},
|
||||
).oauth?.requestRefreshToken,
|
||||
).toBe(false);
|
||||
// On (the default) forwards nothing, so the provider keeps its own default.
|
||||
expect(
|
||||
buildRunnerClientAuthOptions({}, base).oauth?.requestRefreshToken,
|
||||
).toBeUndefined();
|
||||
});
|
||||
|
||||
// #2018 — the CLI/TUI leg picks the authorization parameters up from the same
|
||||
// per-server settings the web leg reads, so all three clients agree.
|
||||
it("buildRunnerClientAuthOptions forwards custom authorization params", () => {
|
||||
|
|
|
|||
|
|
@ -1193,6 +1193,68 @@ describe("enterpriseManaged oauth settings", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("oauth.requestRefreshToken (#2068)", () => {
|
||||
const baseSettings = {
|
||||
headers: [],
|
||||
env: [],
|
||||
metadata: {},
|
||||
connectionTimeout: 0,
|
||||
requestTimeout: 0,
|
||||
taskTtl: 60000,
|
||||
maxFetchRequests: 1000,
|
||||
roots: [],
|
||||
};
|
||||
|
||||
it("lifts an explicit false to settings.oauthRequestRefreshToken", () => {
|
||||
const settings = storedFieldsToInspectorSettings({
|
||||
oauth: { clientId: "cid", requestRefreshToken: false },
|
||||
});
|
||||
expect(settings?.oauthRequestRefreshToken).toBe(false);
|
||||
});
|
||||
|
||||
it("leaves the setting unset when the field is absent (default: on)", () => {
|
||||
const settings = storedFieldsToInspectorSettings({
|
||||
oauth: { clientId: "cid" },
|
||||
});
|
||||
expect(settings?.oauthRequestRefreshToken).toBeUndefined();
|
||||
});
|
||||
|
||||
it("leaves the setting unset for an explicit true, which means the default", () => {
|
||||
const settings = storedFieldsToInspectorSettings({
|
||||
oauth: { clientId: "cid", requestRefreshToken: true },
|
||||
});
|
||||
expect(settings?.oauthRequestRefreshToken).toBeUndefined();
|
||||
});
|
||||
|
||||
it("persists the opt-out under oauth on disk", () => {
|
||||
const stored = inspectorSettingsToStoredFields({
|
||||
...baseSettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
});
|
||||
expect(stored.oauth?.requestRefreshToken).toBe(false);
|
||||
});
|
||||
|
||||
it("writes nothing when the setting is on", () => {
|
||||
expect(
|
||||
inspectorSettingsToStoredFields({
|
||||
...baseSettings,
|
||||
oauthRequestRefreshToken: true,
|
||||
}).oauth,
|
||||
).toBeUndefined();
|
||||
expect(inspectorSettingsToStoredFields(baseSettings).oauth).toBeUndefined();
|
||||
});
|
||||
|
||||
it("round-trips the opt-out", () => {
|
||||
const stored = inspectorSettingsToStoredFields({
|
||||
...baseSettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
});
|
||||
expect(
|
||||
storedFieldsToInspectorSettings(stored)?.oauthRequestRefreshToken,
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("oauthOnInsufficientScope (SEP-2350)", () => {
|
||||
const baseSettings = {
|
||||
headers: [],
|
||||
|
|
|
|||
|
|
@ -631,6 +631,14 @@ describe("server.ts supplemental coverage", () => {
|
|||
expect((await res.json()).error).toMatch(/enterpriseManaged/);
|
||||
});
|
||||
|
||||
it("rejects a non-boolean oauthRequestRefreshToken (#2068)", async () => {
|
||||
const res = await postSettings({
|
||||
...base,
|
||||
oauthRequestRefreshToken: "no",
|
||||
});
|
||||
expect((await res.json()).error).toMatch(/oauthRequestRefreshToken/);
|
||||
});
|
||||
|
||||
it("rejects malformed roots", async () => {
|
||||
const res = await postSettings({ ...base, roots: [{ uri: 1 }] });
|
||||
expect((await res.json()).error).toMatch(/roots/);
|
||||
|
|
@ -682,6 +690,7 @@ describe("server.ts supplemental coverage", () => {
|
|||
oauthClientId: "cid",
|
||||
oauthScopes: "a b",
|
||||
enterpriseManaged: true,
|
||||
oauthRequestRefreshToken: false,
|
||||
roots: [{ uri: "file:///x", name: "x" }],
|
||||
});
|
||||
expect(res.status).toBe(200);
|
||||
|
|
@ -824,6 +833,57 @@ describe("server.ts supplemental coverage", () => {
|
|||
}
|
||||
});
|
||||
|
||||
// #2068 — same all-or-nothing rule for the refresh-token opt-out: a
|
||||
// non-boolean drops the whole `oauth` node rather than reaching the
|
||||
// provider, where only an explicit `false` means anything.
|
||||
it("drops oauth whose requestRefreshToken is not a boolean", async () => {
|
||||
const h = await start({
|
||||
seedConfig: JSON.stringify({
|
||||
mcpServers: {
|
||||
srv: {
|
||||
type: "streamable-http",
|
||||
url: "https://x.test/mcp",
|
||||
oauth: { requestRefreshToken: "no" },
|
||||
},
|
||||
},
|
||||
}),
|
||||
});
|
||||
try {
|
||||
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||
const body = (await res.json()) as {
|
||||
mcpServers: Record<string, Record<string, unknown>>;
|
||||
};
|
||||
expect(body.mcpServers.srv).not.toHaveProperty("oauth");
|
||||
} finally {
|
||||
await stop(h);
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps a well-formed requestRefreshToken opt-out on read (#2068)", async () => {
|
||||
const h = await start({
|
||||
seedConfig: JSON.stringify({
|
||||
mcpServers: {
|
||||
srv: {
|
||||
type: "streamable-http",
|
||||
url: "https://x.test/mcp",
|
||||
oauth: { requestRefreshToken: false },
|
||||
},
|
||||
},
|
||||
}),
|
||||
});
|
||||
try {
|
||||
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||
const body = (await res.json()) as {
|
||||
mcpServers: Record<string, Record<string, unknown>>;
|
||||
};
|
||||
expect(body.mcpServers.srv?.oauth).toEqual({
|
||||
requestRefreshToken: false,
|
||||
});
|
||||
} finally {
|
||||
await stop(h);
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps a well-formed authorizationParams record on read", async () => {
|
||||
const h = await start({
|
||||
seedConfig: JSON.stringify({
|
||||
|
|
|
|||
Loading…
Reference in a new issue