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

feat: improve diagnostics, allow custom HTTP options, account for IdP support for scopes #189

Merged
merged 5 commits into from
Mar 11, 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
14 changes: 7 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.7.1",
"@mongodb-js/oidc-mock-provider": "^0.8.0",
"@mongodb-js/prettier-config-devtools": "^1.0.1",
"@mongodb-js/tsconfig-devtools": "^1.0.0",
"@types/chai": "^4.2.21",
Expand Down
25 changes: 25 additions & 0 deletions src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,24 @@ import type {
OIDCRequestFunction,
TypedEventEmitter,
} from './types';
import type { RequestOptions } from 'https';

/** @public */
export type HttpOptions = Partial<
Pick<
RequestOptions,
| 'agent'
| 'ca'
| 'cert'
| 'crl'
| 'headers'
| 'key'
| 'lookup'
| 'passphrase'
| 'pfx'
| 'timeout'
>
>;

/** @public */
export type AuthFlowType = 'auth-code' | 'device-auth';
Expand Down Expand Up @@ -167,6 +185,13 @@ export interface MongoDBOIDCPluginOptions {
* message being emitted but otherwise be ignored.
*/
throwOnIncompatibleSerializedState?: boolean;

/**
* Provide custom HTTP options for individual HTTP calls.
*/
customHttpOptions?:
| HttpOptions
| ((url: string, options: Readonly<HttpOptions>) => HttpOptions);
}

/** @public */
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export type {
RedirectServerRequestHandler,
RedirectServerRequestInfo,
MongoDBOIDCPluginMongoClientOptions,
HttpOptions,
} from './api';

export type {
Expand Down
40 changes: 40 additions & 0 deletions src/log-hook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@ import type { MongoDBOIDCLogEventsMap, TypedEventEmitter } from './types';

/** @public */
export interface MongoLogWriter {
debug?(
c: string,
id: unknown,
ctx: string,
msg: string,
attr?: unknown,
level?: 1 | 2 | 3 | 4 | 5
): void;
info(c: string, id: unknown, ctx: string, msg: string, attr?: unknown): void;
warn(c: string, id: unknown, ctx: string, msg: string, attr?: unknown): void;
error(c: string, id: unknown, ctx: string, msg: string, attr?: unknown): void;
Expand Down Expand Up @@ -269,4 +277,36 @@ export function hookLoggerToMongoLogWriter(
'Missing ID token in IdP response'
);
});

emitter.on('mongodb-oidc-plugin:outbound-http-request', (ev) => {
log.debug?.(
'OIDC-PLUGIN',
mongoLogId(1_002_000_023),
`${contextPrefix}-oidc`,
'Outbound HTTP request',
{ url: redactUrl(ev.url) }
);
});

emitter.on('mongodb-oidc-plugin:inbound-http-request', (ev) => {
log.debug?.(
'OIDC-PLUGIN',
mongoLogId(1_002_000_024),
`${contextPrefix}-oidc`,
'Inbound HTTP request',
{ url: redactUrl(ev.url) }
);
});
}

function redactUrl(url: string): string {
let parsed: URL;
try {
parsed = new URL(url);
} catch {
return '<Invalid URL>';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Would be nice to log.debug? somehow the wrong URL or at least return the invalid url in case there is an issue.

Suggested change
return '<Invalid URL>';
return `<Invalid URL: '${url}'>`;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kmruiz Well ... I've gone back and forth on this, the point of this function is to redact URLs so that sensitive parameters like the authorization code don't end up being logged (and consequently stored to disk), so I wanted to err on the side of caution. You're also obviously right that that makes for bad diagnostics, not having the URL available at all :) I'd lean a bit towards leaving it like this, this condition Should Never Happen™ anyway because we should always be working with valid URLs, but let me know if you feel differently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged this for now, but happy to adjust in a follow-up if you have more thoughts on this :)

}
for (const key of [...parsed.searchParams.keys()])
parsed.searchParams.set(key, '');
return parsed.toString();
}
168 changes: 168 additions & 0 deletions src/plugin.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type {
OIDCCallbackContext,
IdPServerInfo,
OIDCRequestFunction,
OpenBrowserOptions,
} from './';
import { createMongoDBOIDCPlugin, hookLoggerToMongoLogWriter } from './';
import { once } from 'events';
Expand Down Expand Up @@ -32,6 +33,24 @@ import { publicPluginToInternalPluginMap_DoNotUseOutsideOfTests } from './api';
import type { Server as HTTPServer } from 'http';
import { createServer as createHTTPServer } from 'http';
import type { AddressInfo } from 'net';
import type {
OIDCMockProviderConfig,
TokenMetadata,
} from '@mongodb-js/oidc-mock-provider';
import { OIDCMockProvider } from '@mongodb-js/oidc-mock-provider';

// node-fetch@3 is ESM-only...
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
const fetch: typeof import('node-fetch').default = (...args) =>
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
eval("import('node-fetch')").then((fetch: typeof import('node-fetch')) =>
fetch.default(...args)
);

// A 'browser' implementation that just does HTTP requests and ignores the response.
async function fetchBrowser({ url }: OpenBrowserOptions): Promise<void> {
(await fetch(url)).body?.resume();
}

// Shorthand to avoid having to specify `principalName` and `abortSignal`
// if they aren't being used in the first place.
Expand Down Expand Up @@ -308,6 +327,7 @@ describe('OIDC plugin (local OIDC provider)', function () {
expect(serializedData.oidcPluginStateVersion).to.equal(0);
expect(serializedData.state).to.have.lengthOf(1);
expect(serializedData.state[0][0]).to.be.a('string');
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
expect(Object.keys(serializedData.state[0][1]).sort()).to.deep.equal([
'currentTokenSet',
'lastIdTokenClaims',
Expand Down Expand Up @@ -827,6 +847,20 @@ describe('OIDC plugin (local OIDC provider)', function () {
}
});

it('includes a helpful error message when attempting to reach out to invalid issuer', async function () {
try {
await requestToken(plugin, {
clientId: 'clientId',
issuer: 'https://doesnotexist.mongodb.com/',
});
expect.fail('missed exception');
} catch (err: any) {
expect(err.message).to.include(
'Unable to fetch issuer metadata for "https://doesnotexist.mongodb.com/":'
);
}
});

context('with an issuer that reports custom metadata', function () {
let server: HTTPServer;
let response: Record<string, unknown>;
Expand Down Expand Up @@ -1014,3 +1048,137 @@ describe('OIDC plugin (local OIDC provider)', function () {
});
});
});

// eslint-disable-next-line mocha/max-top-level-suites
describe('OIDC plugin (mock OIDC provider)', function () {
let provider: OIDCMockProvider;
let getTokenPayload: OIDCMockProviderConfig['getTokenPayload'];
let additionalIssuerMetadata: OIDCMockProviderConfig['additionalIssuerMetadata'];
let receivedHttpRequests: string[] = [];
const tokenPayload = {
expires_in: 3600,
payload: {
// Define the user information stored inside the access tokens
groups: ['testgroup'],
sub: 'testuser',
aud: 'resource-server-audience-value',
},
};

before(async function () {
if (+process.version.slice(1).split('.')[0] < 16) {
// JWK support for Node.js KeyObject.export() is only Node.js 16+
// but the OIDCMockProvider implementation needs it.
return this.skip();
}
provider = await OIDCMockProvider.create({
getTokenPayload(metadata: TokenMetadata) {
return getTokenPayload(metadata);
},
additionalIssuerMetadata() {
return additionalIssuerMetadata?.() ?? {};
},
overrideRequestHandler(url: string) {
receivedHttpRequests.push(url);
},
});
});

after(async function () {
await provider?.close?.();
});

beforeEach(function () {
receivedHttpRequests = [];
getTokenPayload = () => tokenPayload;
additionalIssuerMetadata = undefined;
});

context('with different supported built-in scopes', function () {
let getScopes: () => Promise<string[]>;

beforeEach(function () {
getScopes = async function () {
const plugin = createMongoDBOIDCPlugin({
openBrowserTimeout: 60_000,
openBrowser: fetchBrowser,
allowedFlows: ['auth-code'],
redirectURI: 'http://localhost:0/callback',
});
const result = await requestToken(plugin, {
issuer: provider.issuer,
clientId: 'mockclientid',
requestScopes: [],
});
const accessTokenContents = getJWTContents(result.accessToken);
return String(accessTokenContents.scope).split(' ').sort();
};
});

it('will get a list of built-in OpenID scopes by default', async function () {
additionalIssuerMetadata = undefined;
expect(await getScopes()).to.deep.equal(['offline_access', 'openid']);
});

it('will omit built-in scopes if the IdP does not announce support for them', async function () {
additionalIssuerMetadata = () => ({ scopes_supported: ['openid'] });
expect(await getScopes()).to.deep.equal(['openid']);
});
});

context('HTTP request tracking', function () {
it('will log all outgoing HTTP requests', async function () {
const pluginHttpRequests: string[] = [];
const localServerHttpRequests: string[] = [];
const browserHttpRequests: string[] = [];

const plugin = createMongoDBOIDCPlugin({
openBrowserTimeout: 60_000,
openBrowser: async ({ url }) => {
// eslint-disable-next-line no-constant-condition
while (true) {
browserHttpRequests.push(url);
const response = await fetch(url, { redirect: 'manual' });
response.body?.resume();
const redirectTarget =
response.status >= 300 &&
response.status < 400 &&
response.headers.get('location');
if (redirectTarget)
url = new URL(redirectTarget, response.url).href;
else break;
}
},
allowedFlows: ['auth-code'],
redirectURI: 'http://localhost:0/callback',
});
plugin.logger.on('mongodb-oidc-plugin:outbound-http-request', (ev) =>
pluginHttpRequests.push(ev.url)
);
plugin.logger.on('mongodb-oidc-plugin:inbound-http-request', (ev) =>
localServerHttpRequests.push(ev.url)
);
await requestToken(plugin, {
issuer: provider.issuer,
clientId: 'mockclientid',
requestScopes: [],
});

const removeSearchParams = (str: string) =>
Object.assign(new URL(str), { search: '' }).toString();
const allOutboundRequests = [
...pluginHttpRequests,
...browserHttpRequests,
]
.map(removeSearchParams)
.sort();
const allInboundRequests = [
...localServerHttpRequests,
...receivedHttpRequests,
]
.map(removeSearchParams)
.sort();
expect(allOutboundRequests).to.deep.equal(allInboundRequests);
});
});
});
Loading
Loading