fix(oauth): stop claiming the opt-out removes every offline_access
Copilot review round 2 on #2114. The setting was described as guaranteeing "no offline_access, no forced prompt", which is not what the SDK does. `startAuthorization` appends `prompt=consent` for any `offline_access` in the effective scope and never consults `grant_types`, and `determineScope` skips its augmentation when the scope already carries it. So the prompt still goes out when the user configured `offline_access` in Scopes, or left Scopes blank and the resource advertises it in `scopes_supported`. The flag removes the SDK's *automatic* augmentation and nothing more; the docs, the help text and the type say that now. The form also flags the case a user can act on: opted out while `offline_access` is still listed in Scopes. Whole-token match, so `offline_access_extra` does not trip it. Corrects the Clear stored OAuth state claim too. It clears the local tokens and client information — it is not a revocation and cannot reach the AS. A dynamically registered client re-registers on the next connect (the new registration declaring only `authorization_code`); a preconfigured `oauth.clientId` is reused unchanged. Adds the two missing seam tests, both confirmed to fail without the code they cover: App's connect wiring (deleting the spread fails two of the three) and an /api/servers POST followed by a read-back, since a 200 alone never proved the opt-out persisted. 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:
cliffhall
parent
f6fca59624
commit
fca3f1ce67
6 changed files with 230 additions and 4 deletions
|
|
@ -696,6 +696,104 @@ const fetchLogInstances = (
|
|||
FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] }
|
||||
).__fetchLogInstances;
|
||||
|
||||
// #2068 — the web connection seam for the refresh-token opt-out. The provider
|
||||
// and manager tests start from an already-built OAuth config and the runner test
|
||||
// covers only the CLI/TUI leg, so without this case deleting the
|
||||
// `requestRefreshToken` spread from App's `oauthFromServer` would leave the
|
||||
// checkbox persisted but inert with every other new test still green.
|
||||
describe("App wires the refresh-token opt-out into the client (#2068)", () => {
|
||||
const HTTP_SERVER = {
|
||||
id: "A",
|
||||
name: "PlotRocket",
|
||||
config: { type: "streamable-http", url: "https://api.example.com/mcp" },
|
||||
connection: { status: "disconnected" },
|
||||
};
|
||||
|
||||
function mockServersWith(settings?: Partial<InspectorServerSettings>) {
|
||||
vi.mocked(useServers).mockReturnValue({
|
||||
servers: [
|
||||
{
|
||||
...HTTP_SERVER,
|
||||
...(settings ? { settings } : {}),
|
||||
} as unknown as ServerEntry,
|
||||
],
|
||||
loading: false,
|
||||
error: undefined,
|
||||
refresh: vi.fn().mockResolvedValue(undefined),
|
||||
addServer: addServerSpy,
|
||||
updateServer: updateServerSpy,
|
||||
updateServerSettings: updateServerSettingsSpy,
|
||||
removeServer: vi.fn(),
|
||||
reorderServers: vi.fn(),
|
||||
importSource: vi.fn().mockResolvedValue({ servers: {} }),
|
||||
});
|
||||
}
|
||||
|
||||
/** The `oauth` option the mocked InspectorClient constructor was given. */
|
||||
function constructedOAuth(): { requestRefreshToken?: boolean } | undefined {
|
||||
const call = vi.mocked(McpIndex.InspectorClient).mock.calls[0];
|
||||
const options = call?.[1] as
|
||||
| { oauth?: { requestRefreshToken?: boolean } }
|
||||
| undefined;
|
||||
return options?.oauth;
|
||||
}
|
||||
|
||||
let previousUseServers: typeof useServers | undefined;
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
clientInstances.length = 0;
|
||||
previousUseServers = vi.mocked(useServers).getMockImplementation();
|
||||
vi.mocked(useInspectorClient).mockReturnValue(DEFAULT_USE_INSPECTOR_CLIENT);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
if (previousUseServers) {
|
||||
vi.mocked(useServers).mockImplementation(previousUseServers);
|
||||
}
|
||||
});
|
||||
|
||||
it("passes requestRefreshToken: false when the server opted out", async () => {
|
||||
const user = userEvent.setup();
|
||||
mockServersWith({ oauthRequestRefreshToken: false });
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
expect(constructedOAuth()?.requestRefreshToken).toBe(false);
|
||||
});
|
||||
|
||||
// The default must not send the key at all — the provider's own default is
|
||||
// what declares the grant, and a stray `true` here would mask its removal.
|
||||
it("omits requestRefreshToken when the setting is on", async () => {
|
||||
const user = userEvent.setup();
|
||||
mockServersWith({ oauthScopes: "openid" });
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
const oauth = constructedOAuth();
|
||||
expect(oauth).toBeDefined();
|
||||
expect(oauth).not.toHaveProperty("requestRefreshToken");
|
||||
});
|
||||
|
||||
// The opt-out is the only OAuth field set, so it must be enough on its own to
|
||||
// materialize the `oauth` option — otherwise it is dropped for any server
|
||||
// without credentials or scopes, which is the common case here.
|
||||
it("builds the oauth option from the opt-out alone", async () => {
|
||||
const user = userEvent.setup();
|
||||
mockServersWith({ oauthRequestRefreshToken: false });
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
expect(constructedOAuth()).toEqual({ requestRefreshToken: false });
|
||||
});
|
||||
});
|
||||
|
||||
describe("App failed-connection card border (#1621)", () => {
|
||||
beforeEach(() => {
|
||||
clientInstances.length = 0;
|
||||
|
|
|
|||
|
|
@ -751,6 +751,73 @@ describe("ServerSettingsForm", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("warns when opting out while offline_access is still in Scopes", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{
|
||||
...emptySettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
oauthScopes: "openid offline_access",
|
||||
}}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.getByText("offline_access is still requested"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("does not warn when opted out and Scopes omits offline_access", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{
|
||||
...emptySettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
oauthScopes: "openid",
|
||||
}}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText("offline_access is still requested"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// The scope alone is not a problem — with the grant declared, requesting
|
||||
// offline_access is exactly what the default configuration does.
|
||||
it("does not warn when offline_access is in Scopes but the grant is on", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{ ...emptySettings, oauthScopes: "openid offline_access" }}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText("offline_access is still requested"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// A substring must not trip it — `offline_access_extra` is a different scope.
|
||||
it("matches offline_access as a whole scope token, not a substring", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{
|
||||
...emptySettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
oauthScopes: "openid offline_access_extra",
|
||||
}}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText("offline_access is still requested"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// #2018 — custom authorization-request parameters.
|
||||
it("shows the empty hint for authorization parameters when none are set", () => {
|
||||
renderWithMantine(
|
||||
|
|
|
|||
|
|
@ -530,6 +530,15 @@ export function ServerSettingsForm({
|
|||
? " Not applied while Enterprise-managed authorization is on: that flow authorizes against the enterprise IdP, a different authorization server."
|
||||
: "";
|
||||
|
||||
// #2068 — the opt-out only removes the SDK's *automatic* `offline_access`.
|
||||
// `prompt=consent` keys off the scope, not the grant, so an `offline_access`
|
||||
// the user typed here still forces the prompt the checkbox claims to avoid.
|
||||
// Flag that combination rather than letting it read as a fixed configuration.
|
||||
const scopesIncludeOfflineAccess = (settings.oauthScopes ?? "")
|
||||
.split(/\s+/)
|
||||
.includes("offline_access");
|
||||
const refreshTokenOptedOut = settings.oauthRequestRefreshToken === false;
|
||||
|
||||
const rejectedParamKeys = authorizationParams
|
||||
.map((p) => p.key.trim())
|
||||
.filter((key) => isReservedAuthorizationParam(key));
|
||||
|
|
@ -883,7 +892,7 @@ export function ServerSettingsForm({
|
|||
/>
|
||||
<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. Applies on the next connect. Unchecking it does not revoke a refresh token already issued, nor the grant already registered at the authorization server — use Clear stored OAuth state below for that."
|
||||
description="When checked, the Inspector registers with the refresh_token grant so it can renew access tokens without a new sign-in. Unchecking it stops the SDK adding offline_access to the requested scope, and so stops the prompt=consent it forces — on Microsoft Entra ID that prompt routes a non-admin user into the admin-consent workflow (AADSTS90094) even after a tenant admin has consented. It removes only that automatic scope: an offline_access you configure in Scopes still forces the prompt. Applies on the next connect, and does not revoke a refresh token already issued or reach a registration the authorization server already holds."
|
||||
checked={settings.oauthRequestRefreshToken ?? true}
|
||||
onChange={(e) =>
|
||||
onOAuthChange({
|
||||
|
|
@ -892,6 +901,14 @@ export function ServerSettingsForm({
|
|||
})
|
||||
}
|
||||
/>
|
||||
{refreshTokenOptedOut && scopesIncludeOfflineAccess ? (
|
||||
<Alert color="yellow" title="offline_access is still requested">
|
||||
Scopes above lists offline_access, and prompt=consent is added
|
||||
whenever that scope is requested — regardless of this setting.
|
||||
Remove it from Scopes as well, or the consent prompt this
|
||||
checkbox is meant to avoid still goes out.
|
||||
</Alert>
|
||||
) : null}
|
||||
<Stack gap="xs">
|
||||
<FieldLabel>Additional authorization parameters</FieldLabel>
|
||||
<FieldDescription>
|
||||
|
|
|
|||
|
|
@ -675,6 +675,39 @@ describe("server.ts supplemental coverage", () => {
|
|||
expect(res.status).toBe(200);
|
||||
});
|
||||
|
||||
// #2068 — a 200 only proves the payload validated. Without reading the
|
||||
// saved entry back, deleting the `oauthRequestRefreshToken` line from
|
||||
// `normalizeSettings` leaves every other test green while saves through
|
||||
// this route silently revert to the default.
|
||||
it("persists the refresh-token opt-out through a save", async () => {
|
||||
expect(
|
||||
(await postSettings({ ...base, oauthRequestRefreshToken: false }))
|
||||
.status,
|
||||
).toBe(200);
|
||||
|
||||
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||
const body = (await res.json()) as {
|
||||
mcpServers: Record<
|
||||
string,
|
||||
{ oauth?: { requestRefreshToken?: boolean } }
|
||||
>;
|
||||
};
|
||||
expect(body.mcpServers.srv?.oauth?.requestRefreshToken).toBe(false);
|
||||
});
|
||||
|
||||
it("writes no refresh-token field when the setting is on", async () => {
|
||||
expect(
|
||||
(await postSettings({ ...base, oauthRequestRefreshToken: true }))
|
||||
.status,
|
||||
).toBe(200);
|
||||
|
||||
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||
const body = (await res.json()) as {
|
||||
mcpServers: Record<string, { oauth?: Record<string, unknown> }>;
|
||||
};
|
||||
expect(body.mcpServers.srv?.oauth?.requestRefreshToken).toBeUndefined();
|
||||
});
|
||||
|
||||
it("accepts a fully-populated valid settings payload", async () => {
|
||||
const res = await postSettings({
|
||||
...base,
|
||||
|
|
|
|||
|
|
@ -791,9 +791,14 @@ export interface InspectorServerSettings {
|
|||
* Whether the Inspector declares the `refresh_token` grant in its OAuth
|
||||
* client metadata for this server. `undefined` (the default) means on;
|
||||
* persisted as `oauth.requestRefreshToken` only when explicitly off. Turning
|
||||
* it off also drops the SDK's automatic `offline_access` scope and the
|
||||
* it off drops the SDK's *automatic* `offline_access` scope and so the
|
||||
* `prompt=consent` it forces — the Entra admin-consent failure in #2068.
|
||||
*
|
||||
* Only the automatic one: `startAuthorization` adds `prompt=consent` for any
|
||||
* `offline_access` in the effective scope without consulting `grant_types`,
|
||||
* so a scope the user configured (or one the resource advertises when the
|
||||
* scope field is blank) still forces the prompt.
|
||||
*
|
||||
* Read at connect time like the rest of the OAuth block, so a live client
|
||||
* keeps the metadata it was built with. It changes what the Inspector
|
||||
* declares, not state the authorization server already holds: an existing
|
||||
|
|
|
|||
|
|
@ -193,11 +193,17 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w
|
|||
|
||||
> **Reading an older file.** Before #1910 this field was a `[{ "key": …, "value": … }]` pair array. That shape is still **read**, so an existing `mcp.json` keeps working unchanged; it is never written back, so the field is rewritten as an object the next time the Inspector saves the entry.
|
||||
|
||||
`oauth.requestRefreshToken` (default `true`) controls whether the Inspector declares the `refresh_token` grant when it registers its OAuth client. Set it to `false` — or uncheck **Request refresh token** in Server Settings → Authorization — when that grant costs you more than it buys. Declaring it makes the SDK append `offline_access` to the requested scope whenever the authorization server advertises that scope, and appending `offline_access` in turn makes the SDK append `prompt=consent` to the authorization request. On Microsoft Entra ID, a forced `prompt=consent` from a non-admin user, for an app whose permissions require admin consent, is routed into the admin-consent workflow and rejected with `AADSTS90094` — even after a tenant admin has already granted consent ([#2068](https://github.com/modelcontextprotocol/inspector/issues/2068)). Turning the setting off breaks that chain at its source: no grant, no `offline_access`, no forced prompt. Going forward the Inspector then obtains no new refresh token, so an expired access token needs a fresh interactive sign-in rather than a silent refresh. Only `false` is written to disk, so a server that never touched the setting keeps a minimal entry.
|
||||
`oauth.requestRefreshToken` (default `true`) controls whether the Inspector declares the `refresh_token` grant when it registers its OAuth client. Set it to `false` — or uncheck **Request refresh token** in Server Settings → Authorization — when that grant costs you more than it buys.
|
||||
|
||||
The chain it breaks runs through the SDK. `determineScope()` appends `offline_access` to the requested scope when the authorization server advertises that scope **and** the client metadata declares `refresh_token`; `startAuthorization()` then appends `prompt=consent` whenever `offline_access` is in the effective scope. On Microsoft Entra ID a forced `prompt=consent` from a non-admin user, for an app whose permissions require admin consent, is routed into the admin-consent workflow and rejected with `AADSTS90094` — even after a tenant admin has already granted consent ([#2068](https://github.com/modelcontextprotocol/inspector/issues/2068)). Going forward the Inspector then obtains no new refresh token, so an expired access token needs a fresh interactive sign-in rather than a silent refresh. Only `false` is written to disk, so a server that never touched the setting keeps a minimal entry.
|
||||
|
||||
> **It removes the SDK's *automatic* `offline_access`, and nothing more.** `prompt=consent` keys off `offline_access` being in the effective scope; it never looks at `grant_types`. So the prompt still goes out if `offline_access` reaches that scope by another route — you listed it yourself in **Scopes**, or you left Scopes blank and the resource server's metadata advertises it in `scopes_supported`. Unchecking this box is not on its own a guarantee that the consent prompt is gone: check the Scopes field too. The web form flags the first case inline.
|
||||
|
||||
> **It applies on the next connect, not to the live connection.** OAuth is connection-time configuration here — like the client credentials, the scopes, and the endpoint overrides beside it — so a client that is already connected keeps the metadata it was built with. Reconnect for the change to reach the authorization flow.
|
||||
|
||||
> **Two pieces of state outlive the setting, and neither is cleared by unchecking it.** The registration held at the authorization server still lists the grant, because turning the setting off changes what the Inspector *declares*, not what the AS has already recorded. And a refresh token issued before the opt-out stays usable: the SDK's refresh path posts the stored token with `grant_type=refresh_token` without consulting the client metadata, so such a server can keep refreshing silently and you may not observe the cost described above at all. **Clear stored OAuth state** (Server Settings → Authorization) drops both — the next connect registers afresh and signs in interactively.
|
||||
> **Two pieces of state outlive the setting, and neither is cleared by unchecking it.** A refresh token issued before the opt-out stays usable — the SDK's refresh path posts the stored token with `grant_type=refresh_token` without consulting the client metadata, so such a server can keep refreshing silently and you may not observe the cost described above at all. And the registration already held at the authorization server still lists the grant, because turning the setting off changes what the Inspector *declares*, not what the AS has recorded.
|
||||
>
|
||||
> **Clear stored OAuth state** (Server Settings → Authorization) clears the Inspector's **local** copies — the tokens and the client information — which is enough for the first. It is not a revocation: it cannot reach the authorization server. For the second, what happens next depends on how the client was obtained. A **dynamically registered** client is registered afresh on the next connect, and the new registration declares only `authorization_code`; the old one still exists at the AS, unused. A **preconfigured `oauth.clientId`** is reused as-is, so changing what that client declares is done at the authorization server, not here.
|
||||
|
||||
`oauth.authorizationParams` is a string→string record of extra query parameters merged into the OAuth **authorization request** URL only — never the token request. Use it for provider-specific hints the core specs don't standardize (Keycloak's `kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's `audience`). The protocol-critical parameters — `client_id`, `code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`, `response_type`, `scope`, `state` — are **reserved**: the web form rejects them inline, and any that reach the merge anyway are dropped with a warning rather than overriding what the flow set (overriding them breaks PKCE, the CSRF state binding, or RFC 8707). Edit them in Server Settings → Authorization ("Additional authorization parameters"), beside Scopes.
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue