From 5a1026808e405efd93f84b236300868fa8797d8a Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Thu, 14 Nov 2024 14:21:59 +0100 Subject: [PATCH 1/6] fix: always send a nonce in the auth request --- src/plugin.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/plugin.ts b/src/plugin.ts index 659d701..945ba91 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -657,6 +657,8 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { let client!: BaseClient; let actualRedirectURI!: string; + const nonce = generators.nonce(); + try { await withAbortCheck(signal, async ({ signalCheck, signalPromise }) => { // We mark the operations that we want to allow to result in a fallback @@ -680,6 +682,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { code_challenge: codeChallenge, code_challenge_method: 'S256', state: oidcStateParam, + nonce, }); validateSecureHTTPUrl(authCodeFlowUrl, 'authCodeFlowUrl'); const { localUrl, onAccessed: onLocalUrlAccessed } = @@ -760,6 +763,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { const tokenSet = await client.callback(actualRedirectURI, params, { code_verifier: codeVerifier, state: oidcStateParam, + nonce, }); this.updateStateWithTokenSet(state, tokenSet); } From 60c093e14cbc656bac170a95a01b819ec1c6669a Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Thu, 14 Nov 2024 15:49:15 +0100 Subject: [PATCH 2/6] Bump oidc-mock-provider --- package-lock.json | 15 ++++++++------- package.json | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index 1e7b1d9..937f7a4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18,7 +18,7 @@ "@mongodb-js/eslint-config-devtools": "^0.9.9", "@mongodb-js/mocha-config-devtools": "^1.0.0", "@mongodb-js/monorepo-tools": "^1.1.4", - "@mongodb-js/oidc-mock-provider": "^0.8.0", + "@mongodb-js/oidc-mock-provider": "^0.10.2", "@mongodb-js/prettier-config-devtools": "^1.0.1", "@mongodb-js/tsconfig-devtools": "^1.0.0", "@types/chai": "^4.2.21", @@ -2853,10 +2853,11 @@ "dev": true }, "node_modules/@mongodb-js/oidc-mock-provider": { - "version": "0.8.0", - "resolved": "https://registry.npmjs.org/@mongodb-js/oidc-mock-provider/-/oidc-mock-provider-0.8.0.tgz", - "integrity": "sha512-5CrrdD07AmsRVHmq5bGPmJabGa5YWX/B1GJNlI/oXiYQzmBdPQ0XklpjPGnCEEgkcG7OsEgrRjcDnuofBkdNPg==", + "version": "0.10.2", + "resolved": "https://registry.npmjs.org/@mongodb-js/oidc-mock-provider/-/oidc-mock-provider-0.10.2.tgz", + "integrity": "sha512-mH9tpgqYvF2ZRBbFKta+ziN48V+t/+NPLQoe7nZ8bYbWsGfXY79QKMIElaXlU8HnemnqUbOqBSYuizgs62OxfQ==", "dev": true, + "license": "Apache-2.0", "dependencies": { "yargs": "17.7.2" }, @@ -18603,9 +18604,9 @@ } }, "@mongodb-js/oidc-mock-provider": { - "version": "0.8.0", - "resolved": "https://registry.npmjs.org/@mongodb-js/oidc-mock-provider/-/oidc-mock-provider-0.8.0.tgz", - "integrity": "sha512-5CrrdD07AmsRVHmq5bGPmJabGa5YWX/B1GJNlI/oXiYQzmBdPQ0XklpjPGnCEEgkcG7OsEgrRjcDnuofBkdNPg==", + "version": "0.10.2", + "resolved": "https://registry.npmjs.org/@mongodb-js/oidc-mock-provider/-/oidc-mock-provider-0.10.2.tgz", + "integrity": "sha512-mH9tpgqYvF2ZRBbFKta+ziN48V+t/+NPLQoe7nZ8bYbWsGfXY79QKMIElaXlU8HnemnqUbOqBSYuizgs62OxfQ==", "dev": true, "requires": { "yargs": "17.7.2" diff --git a/package.json b/package.json index f14f140..4c0e0c7 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "@mongodb-js/eslint-config-devtools": "^0.9.9", "@mongodb-js/mocha-config-devtools": "^1.0.0", "@mongodb-js/monorepo-tools": "^1.1.4", - "@mongodb-js/oidc-mock-provider": "^0.8.0", + "@mongodb-js/oidc-mock-provider": "^0.10.2", "@mongodb-js/prettier-config-devtools": "^1.0.1", "@mongodb-js/tsconfig-devtools": "^1.0.0", "@types/chai": "^4.2.21", From 32681c48d4b55ad822b4cb601c608b485b3c9bc6 Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Thu, 14 Nov 2024 15:57:04 +0100 Subject: [PATCH 3/6] Update test for azure integration test --- test/oidc-test-provider.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/test/oidc-test-provider.ts b/test/oidc-test-provider.ts index 8e7a6f7..115eefd 100644 --- a/test/oidc-test-provider.ts +++ b/test/oidc-test-provider.ts @@ -282,10 +282,13 @@ async function waitForTitle( ): Promise { await browser.waitUntil(async () => { const actual = (await browser.$(selector).getText()).trim(); - let matches; - if (typeof expected === 'string') + let matches: boolean; + if (typeof expected === 'string') { matches = actual.toLowerCase() === expected.toLowerCase(); - else matches = expected.test(actual); + } else { + matches = expected.test(actual); + } + if (!matches) { throw new Error(`Wanted title "${String(expected)}", saw "${actual}"`); } @@ -481,7 +484,7 @@ export async function azureBrowserDeviceAuthFlow({ try { const normalizeUserCode = (str: string) => str.replace(/-/g, ''); browser = await spawnBrowser(verificationUrl, true); - await waitForTitle(browser, 'Enter code', 'div[role="heading"]'); + await waitForTitle(browser, /Enter code/, 'div[role="heading"]'); await ensureValue( browser, 'input[name="otc"]', From da87441d4538eb8d8590b9834e2b14b46edb7c19 Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Mon, 18 Nov 2024 23:29:24 +0100 Subject: [PATCH 4/6] Add option to skip nonce in auth code request --- src/api.ts | 8 ++++ src/plugin.spec.ts | 101 ++++++++++++++++++++++++++++++--------------- src/plugin.ts | 4 +- 3 files changed, 78 insertions(+), 35 deletions(-) diff --git a/src/api.ts b/src/api.ts index 6aefeff..464b8e1 100644 --- a/src/api.ts +++ b/src/api.ts @@ -197,6 +197,14 @@ export interface MongoDBOIDCPluginOptions { * broken identity providers. */ passIdTokenAsAccessToken?: boolean; + + /** + * Skip the nonce parameter in the Authorization Code request. This could + * be used to work with providers that don't support the nonce parameter. + * + * Default is `false`. + */ + skipNonceInAuthCodeRequest?: boolean; } /** @public */ diff --git a/src/plugin.spec.ts b/src/plugin.spec.ts index 19b7df9..aca7793 100644 --- a/src/plugin.spec.ts +++ b/src/plugin.spec.ts @@ -77,6 +77,19 @@ async function delay(ms: number) { return await new Promise((resolve) => setTimeout(resolve, ms)); } +function testAuthCodeFlow( + fn: (opts: Partial) => Mocha.Func +): void { + for (let skipNonceInAuthCodeRequest of [true, false]) { + describe(`with skipNonceInAuthCodeRequest: ${skipNonceInAuthCodeRequest}`, function () { + it( + 'can successfully authenticate with auth code flow', + fn({ skipNonceInAuthCodeRequest }) + ); + }); + } +} + describe('OIDC plugin (local OIDC provider)', function () { this.timeout(90_000); @@ -138,18 +151,30 @@ describe('OIDC plugin (local OIDC provider)', function () { plugin = createMongoDBOIDCPlugin(pluginOptions); }); - it('can request tokens through the browser', async function () { - const result = await requestToken( - plugin, - provider.getMongodbOIDCDBInfo() - ); - const accessTokenContents = getJWTContents(result.accessToken); - expect(accessTokenContents.sub).to.equal('testuser'); - expect(accessTokenContents.client_id).to.equal( - provider.getMongodbOIDCDBInfo().clientId - ); - verifySuccessfulAuthCodeFlowLog(await readLog()); - }); + testAuthCodeFlow( + (opts) => + async function () { + pluginOptions = { + ...pluginOptions, + ...opts, + }; + plugin = createMongoDBOIDCPlugin(pluginOptions); + + const result = await requestToken( + plugin, + provider.getMongodbOIDCDBInfo() + ); + const accessTokenContents = getJWTContents(result.accessToken); + expect(accessTokenContents.sub).to.equal('testuser'); + expect(accessTokenContents.client_id).to.equal( + provider.getMongodbOIDCDBInfo().clientId + ); + + const log = await readLog(); + console.log(log); + verifySuccessfulAuthCodeFlowLog(log); + } + ); it('will re-use tokens while they are valid if no username was provided', async function () { const skipAuthAttemptEvent = once( @@ -1017,18 +1042,22 @@ describe('OIDC plugin (local OIDC provider)', function () { }; }); - it('can successfully authenticate with Okta using auth code flow', async function () { - plugin = createMongoDBOIDCPlugin({ - ...defaultOpts, - allowedFlows: ['auth-code'], - openBrowser: (opts) => - oktaBrowserAuthCodeFlow({ ...opts, username, password }), - }); - const result = await requestToken(plugin, metadata); + testAuthCodeFlow( + (opts) => + async function () { + plugin = createMongoDBOIDCPlugin({ + ...defaultOpts, + allowedFlows: ['auth-code'], + openBrowser: (opts) => + oktaBrowserAuthCodeFlow({ ...opts, username, password }), + ...opts, + }); + const result = await requestToken(plugin, metadata); - validateToken(getJWTContents(result.accessToken)); - verifySuccessfulAuthCodeFlowLog(await readLog()); - }); + validateToken(getJWTContents(result.accessToken)); + verifySuccessfulAuthCodeFlowLog(await readLog()); + } + ); it('can successfully authenticate with Okta using device auth flow', async function () { plugin = createMongoDBOIDCPlugin({ @@ -1087,18 +1116,22 @@ describe('OIDC plugin (local OIDC provider)', function () { }; }); - it('can successfully authenticate with Azure using auth code flow', async function () { - plugin = createMongoDBOIDCPlugin({ - ...defaultOpts, - allowedFlows: ['auth-code'], - openBrowser: (opts) => - azureBrowserAuthCodeFlow({ ...opts, username, password }), - }); - const result = await requestToken(plugin, metadata); + testAuthCodeFlow( + (opts) => + async function () { + plugin = createMongoDBOIDCPlugin({ + ...defaultOpts, + allowedFlows: ['auth-code'], + openBrowser: (opts) => + azureBrowserAuthCodeFlow({ ...opts, username, password }), + ...opts, + }); + const result = await requestToken(plugin, metadata); - validateToken(getJWTContents(result.accessToken)); - verifySuccessfulAuthCodeFlowLog(await readLog()); - }); + validateToken(getJWTContents(result.accessToken)); + verifySuccessfulAuthCodeFlowLog(await readLog()); + } + ); it('can successfully authenticate with Azure using device auth flow', async function () { plugin = createMongoDBOIDCPlugin({ diff --git a/src/plugin.ts b/src/plugin.ts index 945ba91..279f713 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -657,7 +657,9 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { let client!: BaseClient; let actualRedirectURI!: string; - const nonce = generators.nonce(); + const nonce = this.options.skipNonceInAuthCodeRequest + ? undefined + : generators.nonce(); try { await withAbortCheck(signal, async ({ signalCheck, signalPromise }) => { From 172664f466ee17bde6abac8a8da3726391a2f72e Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Mon, 18 Nov 2024 23:47:16 +0100 Subject: [PATCH 5/6] Update tests --- src/plugin.spec.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/plugin.spec.ts b/src/plugin.spec.ts index aca7793..3cb05bb 100644 --- a/src/plugin.spec.ts +++ b/src/plugin.spec.ts @@ -159,6 +159,10 @@ describe('OIDC plugin (local OIDC provider)', function () { ...opts, }; plugin = createMongoDBOIDCPlugin(pluginOptions); + let idToken: string | undefined; + plugin.logger.once('mongodb-oidc-plugin:auth-succeeded', (event) => { + idToken = event.tokens.idToken; + }); const result = await requestToken( plugin, @@ -170,9 +174,15 @@ describe('OIDC plugin (local OIDC provider)', function () { provider.getMongodbOIDCDBInfo().clientId ); - const log = await readLog(); - console.log(log); - verifySuccessfulAuthCodeFlowLog(log); + verifySuccessfulAuthCodeFlowLog(await readLog()); + + expect(idToken).to.not.be.undefined; + const idTokenContents = getJWTContents(idToken!); + if (opts.skipNonceInAuthCodeRequest) { + expect(idTokenContents.nonce).to.be.undefined; + } else { + expect(idTokenContents.nonce).to.not.be.undefined; + } } ); From 461d7afdf36425bd856dcee89d3053d01e9bfa7f Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Mon, 18 Nov 2024 23:57:05 +0100 Subject: [PATCH 6/6] Make eslint happy --- src/plugin.spec.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plugin.spec.ts b/src/plugin.spec.ts index 3cb05bb..5b28358 100644 --- a/src/plugin.spec.ts +++ b/src/plugin.spec.ts @@ -80,8 +80,8 @@ async function delay(ms: number) { function testAuthCodeFlow( fn: (opts: Partial) => Mocha.Func ): void { - for (let skipNonceInAuthCodeRequest of [true, false]) { - describe(`with skipNonceInAuthCodeRequest: ${skipNonceInAuthCodeRequest}`, function () { + for (const skipNonceInAuthCodeRequest of [true, false]) { + describe(`with skipNonceInAuthCodeRequest: ${skipNonceInAuthCodeRequest.toString()}`, function () { it( 'can successfully authenticate with auth code flow', fn({ skipNonceInAuthCodeRequest }) @@ -177,6 +177,8 @@ describe('OIDC plugin (local OIDC provider)', function () { verifySuccessfulAuthCodeFlowLog(await readLog()); expect(idToken).to.not.be.undefined; + + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- we know it's non-null from the above check const idTokenContents = getJWTContents(idToken!); if (opts.skipNonceInAuthCodeRequest) { expect(idTokenContents.nonce).to.be.undefined;