From 74fbc57a271cfa2a37602395e070602818740d23 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 Apr 2025 14:18:26 +0200 Subject: [PATCH 1/3] fix: use ID token expiry if `passIdTokenAsAccessToken` is set MONGOSH-2145 --- src/plugin.spec.ts | 54 ++++++++++++++++++++++++++++++++++++++----- src/plugin.ts | 57 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 91 insertions(+), 20 deletions(-) diff --git a/src/plugin.spec.ts b/src/plugin.spec.ts index 5b28358..ce664e6 100644 --- a/src/plugin.spec.ts +++ b/src/plugin.spec.ts @@ -905,24 +905,66 @@ describe('OIDC plugin (local OIDC provider)', function () { describe('automaticRefreshTimeoutMS', function () { it('returns the correct automatic refresh timeout', function () { + const now = () => Date.now() / 1000; expect(automaticRefreshTimeoutMS({})).to.equal(undefined); - expect(automaticRefreshTimeoutMS({ expires_in: 10000 })).to.equal( + expect(automaticRefreshTimeoutMS({ expires_at: now() + 10000 })).to.equal( undefined ); expect( - automaticRefreshTimeoutMS({ refresh_token: 'asdf', expires_in: 10000 }) + automaticRefreshTimeoutMS({ + refresh_token: 'asdf', + expires_at: now() + 10000, + }) ).to.equal(9700000); expect( - automaticRefreshTimeoutMS({ refresh_token: 'asdf', expires_in: 100 }) + automaticRefreshTimeoutMS({ + refresh_token: 'asdf', + expires_at: now() + 100, + }) ).to.equal(50000); expect( - automaticRefreshTimeoutMS({ refresh_token: 'asdf', expires_in: 10 }) + automaticRefreshTimeoutMS( + { + refresh_token: 'asdf', + expires_at: now() + 100, + id_token: '...', + claims() { + return { exp: now() + 500 }; + }, + }, + true + ) + ).to.equal(250000); + expect( + automaticRefreshTimeoutMS( + { + refresh_token: 'asdf', + expires_at: now() + 100, + id_token: '...', + claims() { + return { exp: now() + 500 }; + }, + }, + false + ) + ).to.equal(50000); + expect( + automaticRefreshTimeoutMS({ + refresh_token: 'asdf', + expires_at: now() + 10, + }) ).to.equal(undefined); expect( - automaticRefreshTimeoutMS({ refresh_token: 'asdf', expires_in: 0 }) + automaticRefreshTimeoutMS({ + refresh_token: 'asdf', + expires_at: now() + 0, + }) ).to.equal(undefined); expect( - automaticRefreshTimeoutMS({ refresh_token: 'asdf', expires_in: -10 }) + automaticRefreshTimeoutMS({ + refresh_token: 'asdf', + expires_at: now() + -10, + }) ).to.equal(undefined); }); }); diff --git a/src/plugin.ts b/src/plugin.ts index 279f713..1c77522 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -123,27 +123,47 @@ async function getDefaultOpenBrowser(): Promise< }; } +// Trimmed-down type for easier testing +type TokenSetExpiryInfo = Pick< + TokenSet, + 'refresh_token' | 'id_token' | 'expires_at' +> & { + claims?: () => { exp: number }; +}; + +function tokenExpiryInSeconds( + tokenSet: TokenSetExpiryInfo = {}, + passIdTokenAsAccessToken = false +): number { + // If we have an ID token and are supposed to use it, its `exp` claim + // specifies the token expiry. Otherwise, we assume that the `expires_at` + // value presented by the OIDC provider is correct, since OIDC clients are + // not supposed to inspect access tokens and treat them as opaque. + const expiresAt = + (tokenSet.id_token && + passIdTokenAsAccessToken && + tokenSet.claims?.().exp) || + tokenSet.expires_at || + 0; + return Math.max(0, (expiresAt ?? 0) - Date.now() / 1000); +} + /** @internal Exported for testing only */ export function automaticRefreshTimeoutMS( - tokenSet: Pick + tokenSet: TokenSetExpiryInfo, + passIdTokenAsAccessToken = false ): number | undefined { + const expiresIn = tokenExpiryInSeconds(tokenSet, passIdTokenAsAccessToken); + if (!tokenSet.refresh_token || !expiresIn) return; + // If the tokens expire in more than 1 minute, automatically register // a refresh handler. (They should not expire in less; however, // if we didn't handle that case, we'd run the risk of refreshing very // frequently.) Refresh the token 5 minutes before expiration or // halfway between now and the expiration time, whichever comes later // (expires in 1 hour -> refresh in 55 min, expires in 5 min -> refresh in 2.5 min). - if ( - tokenSet.refresh_token && - tokenSet.expires_in && - tokenSet.expires_in >= 60 /* 1 minute */ - ) { - return ( - Math.max( - tokenSet.expires_in - 300 /* 5 minutes */, - tokenSet.expires_in / 2 - ) * 1000 - ); + if (expiresIn >= 60 /* 1 minute */) { + return Math.max(expiresIn - 300 /* 5 minutes */, expiresIn / 2) * 1000; } } @@ -565,7 +585,10 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { const refreshTokenId = getRefreshTokenId(tokenSet.refresh_token); const updateId = updateIdCounter++; - const timerDuration = automaticRefreshTimeoutMS(tokenSet); + const timerDuration = automaticRefreshTimeoutMS( + tokenSet, + this.options.passIdTokenAsAccessToken + ); // Use `.call()` because in browsers, `setTimeout()` requires that it is called // without a `this` value. `.unref()` is not available in browsers either. if (state.timer) this.timers.clearTimeout.call(null, state.timer); @@ -819,7 +842,13 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { try { get_tokens: { - if ((state.currentTokenSet?.set?.expires_in ?? 0) > 5 * 60) { + if ( + tokenExpiryInSeconds( + state.currentTokenSet?.set, + passIdTokenAsAccessToken + ) > + 5 * 60 + ) { this.logger.emit('mongodb-oidc-plugin:skip-auth-attempt', { reason: 'not-expired', }); From b4e59a262f74c645cde6fc3bcd0e9b79e5be95cd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 Apr 2025 15:46:11 +0200 Subject: [PATCH 2/3] fix: do not return token to driver which is being rejected MONGOSH-2147 Ensure that we do not return tokens from a token set that the driver is currently rejecting (in the sense of calling the OIDC callback again and referring to it via the driver `refreshToken` property). --- src/log-hook.ts | 13 ++++++- src/plugin.spec.ts | 92 +++++++++++++++++++++++++++++++++++++++++++++- src/plugin.ts | 71 ++++++++++++++++++++++++++++------- src/types.ts | 7 ++++ src/util.ts | 17 +++++++++ 5 files changed, 185 insertions(+), 15 deletions(-) diff --git a/src/log-hook.ts b/src/log-hook.ts index 1d000c7..ec53c78 100644 --- a/src/log-hook.ts +++ b/src/log-hook.ts @@ -241,7 +241,15 @@ export function hookLoggerToMongoLogWriter( emitter.on( 'mongodb-oidc-plugin:auth-succeeded', - ({ tokenType, refreshToken, expiresAt, passIdTokenAsAccessToken }) => { + ({ + tokenType, + refreshToken, + expiresAt, + passIdTokenAsAccessToken, + forceRefreshOrReauth, + willRetryWithForceRefreshOrReauth, + tokenSetId, + }) => { log.info( 'OIDC-PLUGIN', mongoLogId(1_002_000_017), @@ -252,6 +260,9 @@ export function hookLoggerToMongoLogWriter( refreshToken, expiresAt, passIdTokenAsAccessToken, + forceRefreshOrReauth, + willRetryWithForceRefreshOrReauth, + tokenSetId, } ); } diff --git a/src/plugin.spec.ts b/src/plugin.spec.ts index ce664e6..a69e50c 100644 --- a/src/plugin.spec.ts +++ b/src/plugin.spec.ts @@ -56,13 +56,15 @@ function requestToken( plugin: MongoDBOIDCPlugin, oidcParams: IdPServerInfo, abortSignal?: OIDCAbortSignal, - username?: string + username?: string, + refreshToken?: string ): ReturnType { return plugin.mongoClientOptions.authMechanismProperties.OIDC_HUMAN_CALLBACK({ timeoutContext: abortSignal, version: 1, idpInfo: oidcParams, username, + refreshToken, }); } @@ -390,6 +392,7 @@ describe('OIDC plugin (local OIDC provider)', function () { // eslint-disable-next-line @typescript-eslint/no-unsafe-argument expect(Object.keys(serializedData.state[0][1]).sort()).to.deep.equal([ 'currentTokenSet', + 'discardingTokenSets', 'lastIdTokenClaims', 'serverOIDCMetadata', ]); @@ -1277,6 +1280,93 @@ describe('OIDC plugin (mock OIDC provider)', function () { }); }); + context('when drivers re-request tokens early', function () { + let plugin: MongoDBOIDCPlugin; + + beforeEach(function () { + plugin = createMongoDBOIDCPlugin({ + openBrowserTimeout: 60_000, + openBrowser: fetchBrowser, + allowedFlows: ['auth-code'], + redirectURI: 'http://localhost:0/callback', + }); + }); + + it('will return a different token even if the existing one is not yet expired', async function () { + const result1 = await requestToken(plugin, { + issuer: provider.issuer, + clientId: 'mockclientid', + requestScopes: [], + }); + const result2 = await requestToken(plugin, { + issuer: provider.issuer, + clientId: 'mockclientid', + requestScopes: [], + }); + const result3 = await requestToken( + plugin, + { + issuer: provider.issuer, + clientId: 'mockclientid', + requestScopes: [], + }, + undefined, + undefined, + result2.refreshToken + ); + const result4 = await requestToken( + plugin, + { + issuer: provider.issuer, + clientId: 'mockclientid', + requestScopes: [], + }, + undefined, + undefined, + result2.refreshToken + ); + expect(result1).to.deep.equal(result2); + expect(result2.accessToken).not.to.equal(result3.accessToken); + expect(result2.refreshToken).not.to.equal(result3.refreshToken); + expect(result3).to.deep.equal(result4); + }); + + it('will return only one new token per expired token even when called in parallel', async function () { + const result1 = await requestToken(plugin, { + issuer: provider.issuer, + clientId: 'mockclientid', + requestScopes: [], + }); + const [result2, result3] = await Promise.all([ + requestToken( + plugin, + { + issuer: provider.issuer, + clientId: 'mockclientid', + requestScopes: [], + }, + undefined, + undefined, + result1.refreshToken + ), + requestToken( + plugin, + { + issuer: provider.issuer, + clientId: 'mockclientid', + requestScopes: [], + }, + undefined, + undefined, + result1.refreshToken + ), + ]); + expect(result1.accessToken).not.to.equal(result2.accessToken); + expect(result1.refreshToken).not.to.equal(result2.refreshToken); + expect(result2).to.deep.equal(result3); + }); + }); + context('HTTP request tracking', function () { it('will log all outgoing HTTP requests', async function () { const pluginHttpRequests: string[] = []; diff --git a/src/plugin.ts b/src/plugin.ts index 1c77522..1a951c3 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -10,6 +10,7 @@ import { MongoDBOIDCError } from './types'; import { errorString, getRefreshTokenId, + getStableTokenSetId, messageFromError, normalizeObject, throwIfAborted, @@ -69,6 +70,10 @@ interface UserOIDCAuthState { // A cached Client instance that uses the issuer metadata as discovered // through serverOIDCMetadata. client?: Client; + // A set of refresh token IDs which are currently being rejected, i.e. + // where the driver has called our callback indicating that the corresponding + // access token has become invalid. + discardingTokenSets?: string[]; } // eslint-disable-next-line @typescript-eslint/consistent-type-imports @@ -239,6 +244,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { lastIdTokenClaims: serializedState.lastIdTokenClaims ? { ...serializedState.lastIdTokenClaims } : undefined, + discardingTokenSets: serializedState.discardingTokenSets, }; this.updateStateWithTokenSet( state, @@ -274,6 +280,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { lastIdTokenClaims: state.lastIdTokenClaims ? { ...state.lastIdTokenClaims } : undefined, + discardingTokenSets: state.discardingTokenSets ?? [], }, ] as const; }), @@ -652,6 +659,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { this.logger.emit('mongodb-oidc-plugin:state-updated', { updateId, timerDuration, + tokenSetId: getStableTokenSetId(tokenSet), }); } @@ -821,7 +829,8 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { private async initiateAuthAttempt( state: UserOIDCAuthState, - driverAbortSignal?: OIDCAbortSignal + driverAbortSignal?: OIDCAbortSignal, + { forceRefreshOrReauth = false } = {} ): Promise { throwIfAborted(this.options.signal); throwIfAborted(driverAbortSignal); @@ -843,11 +852,12 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { try { get_tokens: { if ( + !forceRefreshOrReauth && tokenExpiryInSeconds( state.currentTokenSet?.set, passIdTokenAsAccessToken ) > - 5 * 60 + 5 * 60 ) { this.logger.emit('mongodb-oidc-plugin:skip-auth-attempt', { reason: 'not-expired', @@ -915,6 +925,13 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { const { token_type, expires_at, access_token, id_token, refresh_token } = state.currentTokenSet.set; + const tokenSetId = getStableTokenSetId(state.currentTokenSet.set); + + // We would not want to return the access token or ID token of a token set whose + // accompanying refresh token was passed to us by + const willRetryWithForceRefreshOrReauth = + !forceRefreshOrReauth && + !!state.discardingTokenSets?.includes(tokenSetId); this.logger.emit('mongodb-oidc-plugin:auth-succeeded', { tokenType: token_type ?? null, // DPoP or Bearer @@ -926,11 +943,20 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { idToken: id_token, refreshToken: refresh_token, }, + tokenSetId, + forceRefreshOrReauth, + willRetryWithForceRefreshOrReauth, }); + if (willRetryWithForceRefreshOrReauth) { + return await this.initiateAuthAttempt(state, driverAbortSignal, { + forceRefreshOrReauth: true, + }); + } + return { accessToken: passIdTokenAsAccessToken ? id_token || '' : access_token, - refreshToken: refresh_token, + refreshToken: tokenSetId, // Passing `expiresInSeconds: 0` results in the driver not caching the token. // We perform our own caching here inside the plugin, so interactions with the // cache of the driver are not really required or necessarily helpful. @@ -969,20 +995,39 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { username: params.username, }); - if (state.currentAuthAttempt) { - return await state.currentAuthAttempt; + // If the driver called us with a refresh token, that means that its corresponding + // access token has become invalid and we should always return a new one. + if (params.refreshToken) { + (state.discardingTokenSets ??= []).push(params.refreshToken); + this.logger.emit('mongodb-oidc-plugin:discarding-token-set', { + tokenSetId: params.refreshToken, + }); } - const newAuthAttempt = this.initiateAuthAttempt( - state, - params.timeoutContext - ); try { - state.currentAuthAttempt = newAuthAttempt; - return await newAuthAttempt; + if (state.currentAuthAttempt) { + return await state.currentAuthAttempt; + } + + const newAuthAttempt = this.initiateAuthAttempt( + state, + params.timeoutContext + ); + try { + state.currentAuthAttempt = newAuthAttempt; + return await newAuthAttempt; + } finally { + if (state.currentAuthAttempt === newAuthAttempt) + state.currentAuthAttempt = null; + } } finally { - if (state.currentAuthAttempt === newAuthAttempt) - state.currentAuthAttempt = null; + if (params.refreshToken) { + const index = + state.discardingTokenSets?.indexOf(params.refreshToken) ?? -1; + if (index > 0) { + state.discardingTokenSets?.splice(index, 1); + } + } } } diff --git a/src/types.ts b/src/types.ts index af20bd7..7ad6715 100644 --- a/src/types.ts +++ b/src/types.ts @@ -5,6 +5,7 @@ export interface MongoDBOIDCLogEventsMap { }) => void; 'mongodb-oidc-plugin:state-updated': (event: { updateId: number; + tokenSetId: string; timerDuration: number | undefined; }) => void; 'mongodb-oidc-plugin:local-redirect-accessed': (event: { @@ -82,6 +83,12 @@ export interface MongoDBOIDCLogEventsMap { idToken: string | undefined; refreshToken: string | undefined; }; + forceRefreshOrReauth: boolean; + willRetryWithForceRefreshOrReauth: boolean; + tokenSetId: string; + }) => void; + 'mongodb-oidc-plugin:discarding-token-set': (event: { + tokenSetId: string; }) => void; 'mongodb-oidc-plugin:destroyed': () => void; 'mongodb-oidc-plugin:missing-id-token': () => void; diff --git a/src/util.ts b/src/util.ts index 427204d..43e8f5c 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1,3 +1,4 @@ +import type { TokenSet } from 'openid-client'; import type { OIDCAbortSignal } from './types'; import { createHash, randomBytes } from 'crypto'; @@ -135,3 +136,19 @@ export function getRefreshTokenId( 'debugid:' + createHash('sha256').update(salt).update(token).digest('hex') ); } + +export function getStableTokenSetId(tokenSet: TokenSet): string { + const { access_token, id_token, refresh_token, token_type, expires_at } = + tokenSet; + return createHash('sha256') + .update( + JSON.stringify({ + access_token, + id_token, + refresh_token, + token_type, + expires_at, + }) + ) + .digest('hex'); +} From 5de0231ef6781a202cb20c1ef8a43006684bb820 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 Apr 2025 17:25:45 +0200 Subject: [PATCH 3/3] fixup: reduce test flakiness --- test/oidc-test-provider.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/oidc-test-provider.ts b/test/oidc-test-provider.ts index 115eefd..293f312 100644 --- a/test/oidc-test-provider.ts +++ b/test/oidc-test-provider.ts @@ -335,10 +335,18 @@ export async function functioningAuthCodeBrowserFlow({ await ensureValue(browser, 'input[name="login"]', 'testuser'); await ensureValue(browser, 'input[name="password"]', 'testpassword'); await browser.$('button[type="submit"]').click(); + const idpUrl = await browser.getUrl(); await waitForTitle(browser, 'Authorize'); await browser.$('button[type="submit"][autofocus]').click(); - await waitForLocalhostRedirect(browser); + + // Cannot use `waitForLocalhostRedirect` because we already started on localhost + await browser.waitUntil(async () => { + return ( + new URL((await browser?.getUrl()) ?? 'http://nonexistent').host !== + new URL(idpUrl).host + ); + }); } catch (err: unknown) { await dumpHtml(browser); throw err; @@ -385,6 +393,7 @@ export async function functioningDeviceAuthBrowserFlow({ await waitForTitle(browser, 'Authorize'); await browser.$('button[type="submit"][autofocus]').click(); + await waitForTitle(browser, 'Sign-in Success'); } catch (err: unknown) { await dumpHtml(browser); throw err;