fix(oauth): drop an inherited offline_access when the grant is declined
Copilot review round 4 on #2114 — a real gap, not a wording one. Declining the grant stops the SDK *adding* `offline_access`, but the Inspector's own storage puts it back. A successful default-on authorization persists the scope the AS granted (`resolvePersistedScopeAfterGrant`), `offline_access` included; `prepareForAuth` reloads it and `authenticate` hands it to the SDK as `scope`, which sees the token in scope and appends `prompt=consent` again. So any server that had authorized once could uncheck the box and still hit AADSTS90094 — the users most likely to reach for the setting were the ones it did not help. `scopeForDeclinedRefreshGrant` filters it out of the *request* only, applied in the provider's `scope` getter — the one seam both `clientMetadata.scope` and `authenticate`'s `scope:` argument read through. An `offline_access` the user configured is honored; storage is never rewritten, so re-checking the box restores the old behavior instead of finding the value destroyed, and scope-satisfaction checks still see the real grant. Verified by reverting the filter: the new e2e regression fails. Also from that round, both previously suppressed: The checkbox looked effective under enterprise-managed authorization, which never builds this provider at all — it authorizes against the enterprise IdP with a fixed `openid offline_access` (`ema/constants.ts:16`). It now annotates itself the way the endpoint overrides do. The existing count assertion covered two such notices and was named for the overrides alone; it is renamed and expects three, with a dedicated case pinning the notice to this control so the count cannot be met by three copies elsewhere. And "obtains no new refresh token" was another overstatement — the AS decides issuance, and a preconfigured client is reused with whatever its registration already permits. Replaced with what the setting actually controls. 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
6303f8a6ce
commit
89dedd8dea
5 changed files with 323 additions and 4 deletions
|
|
@ -818,6 +818,33 @@ 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.
|
||||
it("says the refresh-token setting is not applied under EMA", () => {
|
||||
const { rerender } = renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{ ...emptySettings, enterpriseManaged: true }}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.getByText(/which this setting does not reach/),
|
||||
).toBeInTheDocument();
|
||||
|
||||
rerender(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{ ...emptySettings, enterpriseManaged: false }}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText(/which this setting does not reach/),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// #2018 — custom authorization-request parameters.
|
||||
it("shows the empty hint for authorization parameters when none are set", () => {
|
||||
renderWithMantine(
|
||||
|
|
@ -1110,7 +1137,7 @@ describe("ServerSettingsForm", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("says the endpoint overrides are unused under enterprise-managed auth", () => {
|
||||
it("says the EMA-suppressed controls are unused under enterprise-managed auth", () => {
|
||||
const { rerender } = renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
|
|
@ -1118,11 +1145,15 @@ describe("ServerSettingsForm", () => {
|
|||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
// Three controls carry this notice: the two endpoint overrides (#1906) and
|
||||
// the refresh-token checkbox (#2068). The count is asserted rather than
|
||||
// merely "present" so a control that stops annotating itself — or a new one
|
||||
// that never starts — is caught here.
|
||||
expect(
|
||||
screen.getAllByText(
|
||||
/Not applied while Enterprise-managed authorization is on/,
|
||||
),
|
||||
).toHaveLength(2);
|
||||
).toHaveLength(3);
|
||||
|
||||
rerender(
|
||||
<ServerSettingsForm
|
||||
|
|
|
|||
|
|
@ -539,6 +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.
|
||||
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."
|
||||
: "";
|
||||
|
||||
const rejectedParamKeys = authorizationParams
|
||||
.map((p) => p.key.trim())
|
||||
.filter((key) => isReservedAuthorizationParam(key));
|
||||
|
|
@ -892,7 +901,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. 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."
|
||||
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.${refreshTokenEmaNote}`}
|
||||
checked={settings.oauthRequestRefreshToken ?? true}
|
||||
onChange={(e) =>
|
||||
onOAuthChange({
|
||||
|
|
|
|||
|
|
@ -506,6 +506,81 @@ describe("OAuthNavigation", () => {
|
|||
makeProviderWithRefresh(false).clientMetadata.grant_types,
|
||||
).toEqual(["authorization_code"]);
|
||||
});
|
||||
|
||||
// The regression that makes the opt-out actually work for a server that
|
||||
// already authorized once. A successful default-on grant persists the
|
||||
// AS-granted scope — `offline_access` included — and the provider reloads
|
||||
// it here. Handing that straight back to the SDK puts the token in scope
|
||||
// again, and `startAuthorization` appends `prompt=consent` off the scope
|
||||
// alone, so the box would be unchecked and AADSTS90094 would persist.
|
||||
describe("scope inherited from an earlier default-on grant", () => {
|
||||
function makeProviderWithScopes(
|
||||
storedScope: string,
|
||||
configuredScope: string | undefined,
|
||||
requestRefreshToken: boolean,
|
||||
): { provider: BaseOAuthClientProvider; storage: OAuthStorage } {
|
||||
const storage = makeStorage();
|
||||
vi.mocked(storage.getScope).mockResolvedValue(storedScope);
|
||||
const provider = new BaseOAuthClientProvider(SERVER, {
|
||||
storage,
|
||||
redirectUrlProvider: new MutableRedirectUrlProvider(),
|
||||
navigation: new CallbackNavigation(vi.fn()),
|
||||
requestRefreshToken,
|
||||
configuredScope,
|
||||
});
|
||||
return { provider, storage };
|
||||
}
|
||||
|
||||
it("drops the inherited offline_access from the requested scope", async () => {
|
||||
const { provider } = makeProviderWithScopes(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
false,
|
||||
);
|
||||
await provider.prepareForAuth();
|
||||
|
||||
expect(provider.scope).toBe("mcp");
|
||||
expect(provider.clientMetadata.scope).toBe("mcp");
|
||||
});
|
||||
|
||||
it("honors an offline_access the user configured", async () => {
|
||||
const { provider } = makeProviderWithScopes(
|
||||
"mcp offline_access",
|
||||
"mcp offline_access",
|
||||
false,
|
||||
);
|
||||
await provider.prepareForAuth();
|
||||
|
||||
expect(provider.scope).toBe("mcp offline_access");
|
||||
});
|
||||
|
||||
it("leaves the scope alone while the grant is declared", async () => {
|
||||
const { provider } = makeProviderWithScopes(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
true,
|
||||
);
|
||||
await provider.prepareForAuth();
|
||||
|
||||
expect(provider.scope).toBe("mcp offline_access");
|
||||
});
|
||||
|
||||
// Filtered at the point of request, never in storage: re-checking the
|
||||
// box must restore the previous behavior rather than find the value
|
||||
// destroyed, and the persisted scope is also what scope-satisfaction
|
||||
// checks read.
|
||||
it("does not rewrite the persisted scope", async () => {
|
||||
const { provider, storage } = makeProviderWithScopes(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
false,
|
||||
);
|
||||
await provider.prepareForAuth();
|
||||
void provider.scope;
|
||||
|
||||
expect(storage.saveScope).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("SEP-2352 issuer threading", () => {
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import {
|
|||
isStrictScopeSuperset,
|
||||
resolveEffectiveGrantedScope,
|
||||
resolvePersistedScopeAfterGrant,
|
||||
scopeForDeclinedRefreshGrant,
|
||||
} from "@inspector/core/auth/scopes.js";
|
||||
|
||||
describe("scopes", () => {
|
||||
|
|
@ -65,3 +66,54 @@ describe("scopes", () => {
|
|||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("scopeForDeclinedRefreshGrant (#2068)", () => {
|
||||
it("drops an inherited offline_access from the requested scope", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("mcp offline_access", "mcp")).toBe(
|
||||
"mcp",
|
||||
);
|
||||
});
|
||||
|
||||
// The persisted scope is what a previous default-on grant left behind, and
|
||||
// the configured scope may be unset entirely (DCR + resource-advertised
|
||||
// scopes). An inherited token is still inherited.
|
||||
it("drops it when no scope is configured at all", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("mcp offline_access", undefined)).toBe(
|
||||
"mcp",
|
||||
);
|
||||
});
|
||||
|
||||
it("keeps an offline_access the user explicitly configured", () => {
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant("mcp offline_access", "mcp offline_access"),
|
||||
).toBe("mcp offline_access");
|
||||
});
|
||||
|
||||
it("leaves a scope without offline_access untouched", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("mcp tools:read", "mcp")).toBe(
|
||||
"mcp tools:read",
|
||||
);
|
||||
});
|
||||
|
||||
// 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", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("offline_access", "")).toBeUndefined();
|
||||
});
|
||||
|
||||
it("passes an absent scope through", () => {
|
||||
expect(scopeForDeclinedRefreshGrant(undefined, "mcp")).toBeUndefined();
|
||||
});
|
||||
|
||||
it("matches whole tokens, not substrings", () => {
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant("mcp offline_access_extra", "mcp"),
|
||||
).toBe("mcp offline_access_extra");
|
||||
});
|
||||
|
||||
it("tolerates irregular whitespace", () => {
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant(" mcp offline_access ", "mcp"),
|
||||
).toBe("mcp");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1459,6 +1459,83 @@ describe("InspectorClient OAuth E2E", () => {
|
|||
// addition, not what the user asked for.
|
||||
expect(scope.split(/\s+/)).toContain("mcp");
|
||||
});
|
||||
|
||||
// #2068 — the case that makes the opt-out worth anything to the user who
|
||||
// reported it: a server that already authorized successfully. That grant
|
||||
// persists the AS-granted scope, `offline_access` included, and the
|
||||
// provider reloads it on the next connect. Handing it back unfiltered puts
|
||||
// the token in scope again and `startAuthorization` re-adds
|
||||
// `prompt=consent` off the scope alone — box unchecked, AADSTS90094 intact.
|
||||
it("drops an offline_access inherited from an earlier grant's stored scope", async () => {
|
||||
const transport = transports[1]!;
|
||||
const staticClientId = "test-inherited-offline-client";
|
||||
const staticClientSecret = "test-inherited-offline-secret";
|
||||
const serverConfig = {
|
||||
...getDefaultServerConfig(),
|
||||
serverType: transport.serverType,
|
||||
...createOAuthTestServerConfig({
|
||||
requireAuth: true,
|
||||
supportRefreshTokens: true,
|
||||
scopesSupported: ["mcp", "offline_access"],
|
||||
staticClients: [
|
||||
{
|
||||
clientId: staticClientId,
|
||||
clientSecret: staticClientSecret,
|
||||
redirectUris: [testRedirectUrl],
|
||||
},
|
||||
],
|
||||
}),
|
||||
};
|
||||
server = new TestServerHttp(serverConfig);
|
||||
const port = await server.start();
|
||||
const serverUrl = `http://localhost:${port}`;
|
||||
await waitForOAuthWellKnown(serverUrl);
|
||||
|
||||
const oauthConfig = createTestOAuthConfig({
|
||||
mode: "static",
|
||||
clientId: staticClientId,
|
||||
clientSecret: staticClientSecret,
|
||||
redirectUrl: testRedirectUrl,
|
||||
scope: "mcp",
|
||||
});
|
||||
// Exactly what a completed default-on authorization leaves behind (see
|
||||
// `resolvePersistedScopeAfterGrant`): the scope the AS granted.
|
||||
const mcpUrl = `${serverUrl}${transport.endpoint}`;
|
||||
await oauthConfig.storage.saveScope(mcpUrl, "mcp offline_access");
|
||||
|
||||
client = new InspectorClient(
|
||||
{ type: transport.clientType, url: mcpUrl } as MCPServerConfig,
|
||||
{
|
||||
environment: {
|
||||
transport: createTransportNode,
|
||||
oauth: {
|
||||
storage: oauthConfig.storage,
|
||||
navigation: oauthConfig.navigation,
|
||||
redirectUrlProvider: oauthConfig.redirectUrlProvider,
|
||||
},
|
||||
},
|
||||
oauth: {
|
||||
clientId: oauthConfig.clientId,
|
||||
clientSecret: oauthConfig.clientSecret,
|
||||
scope: oauthConfig.scope,
|
||||
requestRefreshToken: false,
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
const authUrl = await client.authenticate();
|
||||
if (!authUrl) throw new Error("Expected authorization URL");
|
||||
const scope = authUrl.searchParams.get("scope") ?? "";
|
||||
expect(scope.split(/\s+/)).not.toContain("offline_access");
|
||||
expect(authUrl.searchParams.get("prompt")).toBeNull();
|
||||
expect(scope.split(/\s+/)).toContain("mcp");
|
||||
|
||||
// Filtered on the way out, not in storage — re-checking the box has to
|
||||
// restore the old behavior rather than find the scope destroyed.
|
||||
expect(await oauthConfig.storage.getScope(mcpUrl)).toBe(
|
||||
"mcp offline_access",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("SEP-2352 AS migration (Streamable HTTP)", () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue