diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index a0330516..322cb89b 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -818,9 +818,11 @@ describe("ServerSettingsForm", () => { ).not.toBeInTheDocument(); }); - // #2068 — EMA never builds the Inspector's OAuth provider at all; it - // authorizes against the enterprise IdP with a fixed `openid offline_access`. - // The checkbox therefore cannot affect that request and must say so. + // #2068 — EMA wraps this provider and forwards its `clientMetadata`, so the + // setting is not ignored outright; what it cannot change is the IdP + // authorization leg's fixed `openid offline_access` scope. That leg is the + // one the user signs in through, so the checkbox cannot affect EMA's consent + // behavior and must say so. it("says the refresh-token setting is not applied under EMA", () => { const { rerender } = renderWithMantine( { />, ); expect( - screen.getByText(/which this setting does not reach/), + screen.getByText(/which this setting cannot change/), ).toBeInTheDocument(); rerender( @@ -841,7 +843,7 @@ describe("ServerSettingsForm", () => { />, ); expect( - screen.queryByText(/which this setting does not reach/), + screen.queryByText(/which this setting cannot change/), ).not.toBeInTheDocument(); }); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index 95b5efbe..ae18471f 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -539,13 +539,15 @@ export function ServerSettingsForm({ .includes("offline_access"); const refreshTokenOptedOut = settings.oauthRequestRefreshToken === false; - // #2068 — suppressed under EMA for a different reason than the endpoint - // overrides: that leg never builds a BaseOAuthClientProvider at all. It - // authorizes against the enterprise IdP with its own fixed scope, so this - // checkbox cannot influence the request. Say so rather than leaving a control - // that looks effective, matching how the overrides annotate themselves. + // #2068 — annotated under EMA. Not because the provider is bypassed (EMA + // wraps a BaseOAuthClientProvider and forwards its `clientMetadata`), but + // because the IdP authorization leg — the one the user signs in through — + // uses a fixed `openid offline_access` scope this setting cannot change. The + // control therefore cannot affect EMA's consent behavior, which is the whole + // reason to touch it. Say so rather than leave it looking effective, matching + // how the endpoint overrides annotate themselves. const refreshTokenEmaNote = settings.enterpriseManaged - ? " Not applied while Enterprise-managed authorization is on: that flow authorizes against the enterprise IdP with its own scope, which this setting does not reach." + ? " Not applied while Enterprise-managed authorization is on: that flow signs in against the enterprise IdP using its own fixed scope, which this setting cannot change." : ""; const rejectedParamKeys = authorizationParams diff --git a/clients/web/src/test/core/auth/scopes.test.ts b/clients/web/src/test/core/auth/scopes.test.ts index dfb304d0..b6b248a6 100644 --- a/clients/web/src/test/core/auth/scopes.test.ts +++ b/clients/web/src/test/core/auth/scopes.test.ts @@ -97,8 +97,23 @@ describe("scopeForDeclinedRefreshGrant (#2068)", () => { // Requesting `scope=` empty is not the same as omitting it; collapse to // undefined so the SDK falls back to its own resolution. - it("collapses to undefined when offline_access was the only scope", () => { + it("collapses to undefined when nothing is left and nothing is configured", () => { expect(scopeForDeclinedRefreshGrant("offline_access", "")).toBeUndefined(); + expect( + scopeForDeclinedRefreshGrant("offline_access", undefined), + ).toBeUndefined(); + }); + + // Returning undefined here would read as "nothing stored" to + // OAuthManager.createOAuthProvider, whose seeding branch writes the + // configured scope to storage — turning a request-only filter into a silent + // overwrite. The configured scope is both the right request and inert to that + // branch. + it("falls back to the configured scope rather than emptying out", () => { + expect(scopeForDeclinedRefreshGrant("offline_access", "mcp")).toBe("mcp"); + expect( + scopeForDeclinedRefreshGrant(" offline_access ", " mcp tools:read "), + ).toBe("mcp tools:read"); }); it("passes an absent scope through", () => { diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 192e07d1..d6ec9280 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -1703,6 +1703,28 @@ describe("OAuthManager", () => { ]); }); + // #2068 round 5 — the provider-level test for "storage is never rewritten" + // could not see this: `createOAuthProvider` reads `provider.scope` (the + // filtered getter) and seeds storage when it comes back undefined. A + // persisted scope of `offline_access` alone therefore used to be silently + // overwritten by the configured scope, which is the one thing the filter + // promises not to do. Exercised through the manager, where the seeding + // branch actually lives. + it("does not overwrite a persisted scope that filters down to nothing", async () => { + const params = createMockParams(); + const storage = params.initialConfig.storage; + if (!storage) throw new Error("expected mock storage"); + vi.mocked(storage.getScope).mockResolvedValue("offline_access"); + + const manager = new OAuthManager(params); + manager.setOAuthConfig({ requestRefreshToken: false, scope: "mcp" }); + const provider = await manager.createOAuthProviderForTransport(); + + expect(storage.saveScope).not.toHaveBeenCalled(); + // Still requests the configured scope, without the declined token. + expect(provider.clientMetadata.scope).toBe("mcp"); + }); + it("declares the refresh_token grant when the opt-out is not configured", async () => { const manager = new OAuthManager(createMockParams()); diff --git a/core/auth/scopes.ts b/core/auth/scopes.ts index d658ea8a..ba11a2c1 100644 --- a/core/auth/scopes.ts +++ b/core/auth/scopes.ts @@ -55,9 +55,24 @@ export function scopeForDeclinedRefreshGrant( .trim() .split(/\s+/) .filter((token) => token !== "" && token !== OFFLINE_ACCESS_SCOPE); - // An all-`offline_access` scope collapses to undefined rather than "", so the - // SDK falls back to its own resolution instead of requesting an empty scope. - return kept.length > 0 ? kept.join(" ") : undefined; + if (kept.length > 0) return kept.join(" "); + + // Filtering removed everything (the persisted scope was `offline_access` + // alone). Returning `undefined` here would be read as "nothing stored" by + // `OAuthManager.createOAuthProvider`, whose seeding branch then *writes* the + // configured scope to storage — turning this request-only filter into a + // silent overwrite of the persisted value, so re-enabling the grant could no + // longer restore it. Hand back the configured scope instead: it is the right + // thing to request, and it leaves storage alone. + // Cannot itself contain `offline_access`: that case returned unchanged above. + const fallback = configuredScope?.trim(); + if (fallback) return fallback; + + // Nothing configured either, so there is no value to preserve and the seeding + // branch is inert (it requires a configured scope). `undefined` rather than + // `""` so the SDK falls back to its own resolution instead of requesting an + // empty scope. + return undefined; } /** diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index 9a1fed29..b930dff9 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -201,7 +201,7 @@ The setting suppresses the grant declaration and the SDK's scope augmentation > **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. -> **Not applied under enterprise-managed authorization** (`oauth.enterpriseManaged`) — and for a stronger reason than the endpoint overrides above: that leg never builds the Inspector's OAuth client provider at all. It authorizes against the enterprise IdP with a fixed `openid offline_access` scope, which this setting does not reach. The web form says so inline when EMA is on. +> **Not applied to the enterprise-managed authorization request** (`oauth.enterpriseManaged`). The setting does reach the OAuth client metadata under EMA — that flow wraps the same provider and forwards its metadata — but the leg you actually sign in through authorizes against the enterprise IdP with a fixed `openid offline_access` scope that this setting cannot change. So it cannot affect EMA's consent behavior, which is what the checkbox is for. The web form says so inline when EMA is on. > **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.