Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: always send a nonce in the auth request MONGOSH-1905 #195

Merged
merged 6 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
8 changes: 8 additions & 0 deletions src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
113 changes: 79 additions & 34 deletions src/plugin.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,19 @@ async function delay(ms: number) {
return await new Promise((resolve) => setTimeout(resolve, ms));
}

function testAuthCodeFlow(
fn: (opts: Partial<MongoDBOIDCPluginOptions>) => Mocha.Func
): void {
for (const skipNonceInAuthCodeRequest of [true, false]) {
describe(`with skipNonceInAuthCodeRequest: ${skipNonceInAuthCodeRequest.toString()}`, function () {
it(
'can successfully authenticate with auth code flow',
fn({ skipNonceInAuthCodeRequest })
);
});
}
}

describe('OIDC plugin (local OIDC provider)', function () {
this.timeout(90_000);

Expand Down Expand Up @@ -138,18 +151,42 @@ 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);
let idToken: string | undefined;
plugin.logger.once('mongodb-oidc-plugin:auth-succeeded', (event) => {
idToken = event.tokens.idToken;
});

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());

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;
} else {
expect(idTokenContents.nonce).to.not.be.undefined;
}
}
);

it('will re-use tokens while they are valid if no username was provided', async function () {
const skipAuthAttemptEvent = once(
Expand Down Expand Up @@ -1017,18 +1054,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({
Expand Down Expand Up @@ -1087,18 +1128,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({
Expand Down
6 changes: 6 additions & 0 deletions src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,10 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin {
let client!: BaseClient;
let actualRedirectURI!: string;

const nonce = this.options.skipNonceInAuthCodeRequest
? undefined
: generators.nonce();

try {
await withAbortCheck(signal, async ({ signalCheck, signalPromise }) => {
// We mark the operations that we want to allow to result in a fallback
Expand All @@ -680,6 +684,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin {
code_challenge: codeChallenge,
code_challenge_method: 'S256',
state: oidcStateParam,
nonce,
});
validateSecureHTTPUrl(authCodeFlowUrl, 'authCodeFlowUrl');
const { localUrl, onAccessed: onLocalUrlAccessed } =
Expand Down Expand Up @@ -760,6 +765,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin {
const tokenSet = await client.callback(actualRedirectURI, params, {
code_verifier: codeVerifier,
state: oidcStateParam,
nonce,
});
this.updateStateWithTokenSet(state, tokenSet);
}
Expand Down
11 changes: 7 additions & 4 deletions test/oidc-test-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,13 @@ async function waitForTitle(
): Promise<void> {
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}"`);
}
Expand Down Expand Up @@ -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"]',
Expand Down
Loading