From c6029e1cbbf4c2761c59dd28be87eb1b2999010c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 25 Aug 2026 01:32:40 -0400 Subject: [PATCH] fix(oauth): persist the granted scope on every EMA mint `trySilentEmaAuth` and `refreshEmaResourceTokens` saved tokens without ever saving a scope. Both are reached from real paths -- initial EMA authentication with a cached IdP session, refreshEnterpriseManagedTokens, and EmaTransportOAuthProvider.tokens -- and each can be the moment a newly configured or metadata-derived scope first takes effect, so a silent token response left the previous stored scope standing. Rather than a third copy of the rule, every EMA mint now ends in one place: mintEmaResourceTokens reports the scope it requested, and saveMintedTokens writes tokens and the RFC 6749 5.1 scope together. The manager's EMA branch drops its own saveScope, which would now be a redundant second write of the same value; the step-up branch keeps its own, since it falls back to the challenge's authorizationScopes union and runs last. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- .../src/test/core/auth/ema/emaFlow.test.ts | 47 +++++++++- .../src/test/core/mcp/oauthManager.test.ts | 75 +-------------- core/auth/ema/emaFlow.ts | 93 ++++++++++--------- core/mcp/oauthManager.ts | 25 ++--- 4 files changed, 104 insertions(+), 136 deletions(-) diff --git a/clients/web/src/test/core/auth/ema/emaFlow.test.ts b/clients/web/src/test/core/auth/ema/emaFlow.test.ts index 95c67eab..e5e24e54 100644 --- a/clients/web/src/test/core/auth/ema/emaFlow.test.ts +++ b/clients/web/src/test/core/auth/ema/emaFlow.test.ts @@ -70,6 +70,8 @@ function createMemoryStorage( saveTokens: vi.fn(async (url: string, tokens) => { savedTokens[url] = tokens; }), + getScope: vi.fn(async () => undefined), + saveScope: vi.fn(), clear: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), } as unknown as OAuthStorage; @@ -186,7 +188,7 @@ describe("emaFlow", () => { const idToken = jwtWithExp(exp); idpSessions[IDP_ISSUER] = { idToken }; - const tokens = await mintEmaResourceTokens( + const { tokens } = await mintEmaResourceTokens( { ...baseConfig(storage), fetchFn: mockEmaFetch(idToken) }, resourceContext(), ); @@ -318,7 +320,7 @@ describe("emaFlow", () => { }, ); - const tokens = await mintEmaResourceTokens( + const { tokens } = await mintEmaResourceTokens( { ...baseConfig(storage), fetchFn }, resourceContext(), ); @@ -349,7 +351,7 @@ describe("emaFlow", () => { const idToken = jwtWithExp(exp); idpSessions[IDP_ISSUER] = { idToken }; - const tokens = await mintEmaResourceTokens({ + const { tokens } = await mintEmaResourceTokens({ ...baseConfig(storage), fetchFn: mockEmaFetch(idToken), }); @@ -475,6 +477,45 @@ describe("emaFlow", () => { ); }); + it("trySilentEmaAuth persists the scope its mint asked for (#2117)", async () => { + // The silent path is how an initial EMA authentication completes when the + // IdP session is still cached, so it is a moment a newly configured scope + // first takes effect -- and the mock AS answers without a `scope`. + const idToken = jwtWithExp(Math.floor(Date.now() / 1000) + 3600); + const storage = createMemoryStorage({ [IDP_ISSUER]: { idToken } }); + const fetchFn = mockEmaFetch(idToken); + + const result = await trySilentEmaAuth({ + ...baseConfig(storage), + scope: "mcp weather:read", + fetchFn, + }); + + expect(result.status).toBe("success"); + expect(storage.saveScope).toHaveBeenCalledWith( + SERVER_URL, + "mcp weather:read", + ); + }); + + it("refreshEmaResourceTokens persists the scope its mint asked for (#2117)", async () => { + const idToken = jwtWithExp(Math.floor(Date.now() / 1000) + 3600); + const storage = createMemoryStorage({ [IDP_ISSUER]: { idToken } }); + const fetchFn = mockEmaFetch(idToken); + + const tokens = await refreshEmaResourceTokens({ + ...baseConfig(storage), + scope: "mcp weather:read", + fetchFn, + }); + + expect(tokens?.access_token).toBe("resource-access-token"); + expect(storage.saveScope).toHaveBeenCalledWith( + SERVER_URL, + "mcp weather:read", + ); + }); + it("refreshEmaResourceTokens returns undefined without an IdP session", async () => { expect(await refreshEmaResourceTokens(baseConfig(storage))).toBeUndefined(); }); diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index c8597381..b875f8f5 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -890,74 +890,6 @@ describe("OAuthManager", () => { expect(manager.getOAuthFlowStep()).toBe("complete"); mintSpy.mockRestore(); }); - - it("persists the minted scope request when the mint response omits scope (#2117)", async () => { - const params = createMockParams({ - enterpriseManagedAuth: { - idp: { - issuer: "https://idp.example.com", - clientId: "app-client", - clientSecret: "secret", - }, - }, - }); - params.initialConfig.scope = "mcp weather:read"; - storageOf(params).getScope.mockResolvedValue(undefined); - const manager = new OAuthManager(params); - manager.setOAuthConfig({ enterpriseManaged: true }); - - const mintSpy = vi - .spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint") - .mockResolvedValue({ - tokens: { access_token: "EMA", token_type: "Bearer" }, - requestedScope: "mcp weather:read", - }); - storageOf(params).saveScope.mockClear(); - - await manager.completeOAuthFlow("ema-code"); - - expect(storageOf(params).saveScope).toHaveBeenCalledWith( - SERVER_URL, - "mcp weather:read", - ); - mintSpy.mockRestore(); - }); - - it("persists a mint scope that came from resource metadata, not config (#2117)", async () => { - // With neither a configured nor a stored scope, discoverEmaResourceContext - // resolves the request from the protected resource metadata's - // `scopes_supported`. That value never reaches the manager's own - // `scopeForMint`, so only what the mint reports can be persisted. - const params = createMockParams({ - enterpriseManagedAuth: { - idp: { - issuer: "https://idp.example.com", - clientId: "app-client", - clientSecret: "secret", - }, - }, - }); - params.initialConfig.scope = undefined; - storageOf(params).getScope.mockResolvedValue(undefined); - const manager = new OAuthManager(params); - manager.setOAuthConfig({ enterpriseManaged: true }); - - const mintSpy = vi - .spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint") - .mockResolvedValue({ - tokens: { access_token: "EMA", token_type: "Bearer" }, - requestedScope: "mcp:read mcp:write", - }); - storageOf(params).saveScope.mockClear(); - - await manager.completeOAuthFlow("ema-code"); - - expect(storageOf(params).saveScope).toHaveBeenCalledWith( - SERVER_URL, - "mcp:read mcp:write", - ); - mintSpy.mockRestore(); - }); }); describe("trySilentEnterpriseManagedAuth", () => { @@ -1823,10 +1755,9 @@ describe("OAuthManager", () => { "auth-code", "https://idp.example.com", ); - expect(storageOf(params).saveScope).toHaveBeenCalledWith( - SERVER_URL, - "mcp tools:read weather:read", - ); + // Persisting the granted scope is the EMA flow's job (saveMintedTokens), + // and it is mocked here -- the union reaching its config, asserted just + // above, is what this test owns. emaFlow.test.ts covers the persistence. silentSpy.mockRestore(); startSpy.mockRestore(); diff --git a/core/auth/ema/emaFlow.ts b/core/auth/ema/emaFlow.ts index 475e603f..b1be981e 100644 --- a/core/auth/ema/emaFlow.ts +++ b/core/auth/ema/emaFlow.ts @@ -11,6 +11,7 @@ import { type EmaResourceContext, } from "./resourceContext.js"; import { exchangeIdJag, redeemIdJagForAccessToken } from "./wire.js"; +import { resolvePersistedScopeAfterGrant } from "../scopes.js"; export interface EmaFlowConfig { serverUrl: string; @@ -35,10 +36,24 @@ function wrapEmaMintError(err: unknown): Error { }); } +/** + * A completed EMA mint, and the scope it actually asked for. + * + * The request is not always `config.scope`: with neither a configured nor a + * stored scope, {@link discoverEmaResourceContext} resolves it from the + * protected resource metadata's `scopes_supported`, and that is what both + * exchanges send. Callers need the resolved value to apply RFC 6749 §5.1 — a + * token response that omits `scope` granted exactly what was requested. + */ +export interface EmaMintResult { + tokens: OAuthTokens; + requestedScope?: string; +} + export async function mintEmaResourceTokens( config: EmaFlowConfig, resourceContext?: EmaResourceContext, -): Promise { +): Promise { const ctx = resourceContext ?? (await discoverEmaResourceContext( @@ -78,7 +93,7 @@ export async function mintEmaResourceTokens( fetchFn: config.fetchFn, }); - return redeemIdJagForAccessToken({ + const tokens = await redeemIdJagForAccessToken({ resourceAsUrl: ctx.resourceAsUrl, idJag, resourceClientId: config.resourceClientId, @@ -87,6 +102,31 @@ export async function mintEmaResourceTokens( scope: ctx.scope, fetchFn: config.fetchFn, }); + return { tokens, requestedScope: ctx.scope }; +} + +/** + * Save a mint's tokens and the scope they carry, together. + * + * Every EMA mint ends here — silent, refresh, and interactive alike — because + * all three can be the moment a newly configured or metadata-derived scope + * first takes effect. Persisting tokens without the scope leaves the previous + * stored value standing as though it described the new token (#2117). + */ +async function saveMintedTokens( + config: EmaFlowConfig, + { tokens, requestedScope }: EmaMintResult, +): Promise { + await config.storage.saveTokens(config.serverUrl, tokens, { + enterpriseManaged: true, + }); + const scopeToPersist = resolvePersistedScopeAfterGrant( + tokens.scope, + requestedScope, + ); + if (scopeToPersist) { + await config.storage.saveScope(config.serverUrl, scopeToPersist); + } } /** Silent path: cached IdP session + legs 2–3. */ @@ -102,10 +142,7 @@ export async function trySilentEmaAuth( return { status: "no_idp_session" }; } try { - const tokens = await mintEmaResourceTokens(config); - await config.storage.saveTokens(config.serverUrl, tokens, { - enterpriseManaged: true, - }); + await saveMintedTokens(config, await mintEmaResourceTokens(config)); return { status: "success" }; } catch (err) { return { status: "mint_failed", error: wrapEmaMintError(err) }; @@ -124,26 +161,11 @@ export async function startEmaIdpAuthorization( return authorizationUrl; } -/** - * The outcome of a completed EMA authorization. - * - * `requestedScope` is the scope the mint actually asked for, which is not - * always `config.scope`: when neither the server config nor storage supplies - * one, {@link discoverEmaResourceContext} resolves it from the protected - * resource metadata's `scopes_supported`. Callers need that resolved value to - * apply RFC 6749 §5.1 — a token response that omits `scope` granted exactly - * what was requested — so it is returned rather than left inside the flow. - */ -export interface EmaAuthorizationResult { - tokens: OAuthTokens; - requestedScope?: string; -} - export async function completeEmaIdpAuthorizationAndMint( config: EmaFlowConfig, authorizationCode: string, iss?: string, -): Promise { +): Promise { try { await completeIdpOidcAuthorization({ idp: config.idp, @@ -160,26 +182,15 @@ export async function completeEmaIdpAuthorizationAndMint( }); } - let tokens: OAuthTokens; - // Resolve the resource context here rather than inside the mint, so the - // scope it settles on is visible to the caller. It is passed straight - // through, so this costs no extra discovery. - let resourceContext: EmaResourceContext; + let minted: EmaMintResult; try { - resourceContext = await discoverEmaResourceContext( - config.serverUrl, - config.scope, - config.fetchFn, - ); - tokens = await mintEmaResourceTokens(config, resourceContext); + minted = await mintEmaResourceTokens(config); } catch (err) { throw wrapEmaMintError(err); } - await config.storage.saveTokens(config.serverUrl, tokens, { - enterpriseManaged: true, - }); - return { tokens, requestedScope: resourceContext.scope }; + await saveMintedTokens(config, minted); + return minted; } /** Re-run legs 2–3 on 401 when IdP session is still valid. */ @@ -194,9 +205,7 @@ export async function refreshEmaResourceTokens( if (!idToken) { return undefined; } - const tokens = await mintEmaResourceTokens(config); - await config.storage.saveTokens(config.serverUrl, tokens, { - enterpriseManaged: true, - }); - return tokens; + const minted = await mintEmaResourceTokens(config); + await saveMintedTokens(config, minted); + return minted.tokens; } diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index a1e6eea6..48d278d0 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -376,26 +376,13 @@ export class OAuthManager { const config = scopeForMint ? { ...emaConfig, scope: scopeForMint } : emaConfig; - const { tokens, requestedScope } = - await completeEmaIdpAuthorizationAndMint( - config, - authorizationCode, - iss, - ); - // The mint reports what it actually asked for, which is neither - // `pendingAuthorizationScope` (undefined on the ordinary leg) nor - // always `scopeForMint` — with no configured or stored scope the - // resource metadata's `scopes_supported` is what gets requested. - const scopeToPersist = resolvePersistedScopeAfterGrant( - tokens.scope, - requestedScope, + // The EMA flow persists the granted scope alongside the tokens, for + // every mint rather than only this one — see saveMintedTokens. + const { tokens } = await completeEmaIdpAuthorizationAndMint( + config, + authorizationCode, + iss, ); - if (scopeToPersist) { - await this.requireStorage().saveScope( - this.getServerUrl(), - scopeToPersist, - ); - } this.pendingAuthorizationScope = undefined; const completedAt = Date.now(); this.oauthFlowState = {