Skip to content

Commit 30f2a2d

Browse files
syz99syz99
authored
fix(NODE-3675): SRV option bug correctly defaults authSource to $external (#3079)
1 parent 6eb4edd commit 30f2a2d

File tree

4 files changed

+150
-25
lines changed

4 files changed

+150
-25
lines changed

‎src/cmap/auth/mongo_credentials.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import type { Document } from '../../bson';
44
import { MongoAPIError, MongoMissingCredentialsError } from '../../error';
5-
import { AuthMechanism } from './providers';
5+
import { AUTH_MECHS_AUTH_SRC_EXTERNAL, AuthMechanism } from './providers';
66

77
// https://github.com/mongodb/specifications/blob/master/source/auth/auth.rst
88
function getDefaultAuthMechanism(ismaster?: Document): AuthMechanism {
@@ -136,11 +136,7 @@ export class MongoCredentials {
136136
throw new MongoMissingCredentialsError(`Username required for mechanism '${this.mechanism}'`);
137137
}
138138

139-
if (
140-
this.mechanism === AuthMechanism.MONGODB_GSSAPI ||
141-
this.mechanism === AuthMechanism.MONGODB_AWS ||
142-
this.mechanism === AuthMechanism.MONGODB_X509
143-
) {
139+
if (AUTH_MECHS_AUTH_SRC_EXTERNAL.has(this.mechanism)) {
144140
if (this.source != null && this.source !== '$external') {
145141
// TODO(NODE-3485): Replace this with a MongoAuthValidationError
146142
throw new MongoAPIError(

‎src/cmap/auth/providers.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,10 @@ export const AuthMechanism = Object.freeze({
1212

1313
/** @public */
1414
export type AuthMechanism = typeof AuthMechanism[keyof typeof AuthMechanism];
15+
16+
/** @internal */
17+
export const AUTH_MECHS_AUTH_SRC_EXTERNAL = new Set<AuthMechanism>([
18+
AuthMechanism.MONGODB_GSSAPI,
19+
AuthMechanism.MONGODB_AWS,
20+
AuthMechanism.MONGODB_X509
21+
]);

‎src/connection_string.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { URLSearchParams } from 'url';
55

66
import type { Document } from './bson';
77
import { MongoCredentials } from './cmap/auth/mongo_credentials';
8-
import { AuthMechanism } from './cmap/auth/providers';
8+
import { AUTH_MECHS_AUTH_SRC_EXTERNAL, AuthMechanism } from './cmap/auth/providers';
99
import { Compressor, CompressorName } from './cmap/wire_protocol/compression';
1010
import { Encrypter } from './encrypter';
1111
import { MongoAPIError, MongoInvalidArgumentError, MongoParseError } from './error';
@@ -125,7 +125,12 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback<HostA
125125
const replicaSet = txtRecordOptions.get('replicaSet') ?? undefined;
126126
const loadBalanced = txtRecordOptions.get('loadBalanced') ?? undefined;
127127

128-
if (!options.userSpecifiedAuthSource && source) {
128+
if (
129+
!options.userSpecifiedAuthSource &&
130+
source &&
131+
options.credentials &&
132+
!AUTH_MECHS_AUTH_SRC_EXTERNAL.has(options.credentials.mechanism)
133+
) {
129134
options.credentials = MongoCredentials.merge(options.credentials, { source });
130135
}
131136

@@ -570,9 +575,7 @@ export const OPTIONS = {
570575
let source = options.credentials?.source;
571576
if (
572577
mechanism === AuthMechanism.MONGODB_PLAIN ||
573-
mechanism === AuthMechanism.MONGODB_GSSAPI ||
574-
mechanism === AuthMechanism.MONGODB_AWS ||
575-
mechanism === AuthMechanism.MONGODB_X509
578+
AUTH_MECHS_AUTH_SRC_EXTERNAL.has(mechanism)
576579
) {
577580
// some mechanisms have '$external' as the Auth Source
578581
source = '$external';

‎test/unit/connection_string.test.js renamed to ‎test/unit/connection_string.test.ts

Lines changed: 133 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
1-
'use strict';
2-
3-
const { MongoParseError, MongoDriverError, MongoInvalidArgumentError } = require('../../src/error');
4-
const { loadSpecTests } = require('../spec');
5-
const { parseOptions } = require('../../src/connection_string');
6-
const { AuthMechanism } = require('../../src/cmap/auth/providers');
7-
const { expect } = require('chai');
1+
import { expect } from 'chai';
2+
import * as dns from 'dns';
3+
import * as sinon from 'sinon';
4+
import { promisify } from 'util';
5+
6+
import { MongoCredentials } from '../../src/cmap/auth/mongo_credentials';
7+
import { AUTH_MECHS_AUTH_SRC_EXTERNAL, AuthMechanism } from '../../src/cmap/auth/providers';
8+
import { parseOptions, resolveSRVRecord } from '../../src/connection_string';
9+
import { MongoDriverError, MongoInvalidArgumentError, MongoParseError } from '../../src/error';
10+
import { MongoOptions } from '../../src/mongo_client';
11+
import { loadSpecTests } from '../spec';
812

913
// NOTE: These are cases we could never check for unless we write our own
1014
// url parser. The node parser simply won't let these through, so we
@@ -30,15 +34,17 @@ describe('Connection String', function () {
3034
auth: { user: 'testing', password: 'llamas' }
3135
};
3236

33-
expect(() => parseOptions('mongodb://localhost', optionsWithUser)).to.throw(MongoParseError);
37+
expect(() => parseOptions('mongodb://localhost', optionsWithUser as any)).to.throw(
38+
MongoParseError
39+
);
3440
});
3541

3642
it('should support auth passed with username', function () {
3743
const optionsWithUsername = {
3844
authMechanism: 'SCRAM-SHA-1',
3945
auth: { username: 'testing', password: 'llamas' }
4046
};
41-
const options = parseOptions('mongodb://localhost', optionsWithUsername);
47+
const options = parseOptions('mongodb://localhost', optionsWithUsername as any);
4248
expect(options.credentials).to.containSubset({
4349
source: 'admin',
4450
username: 'testing',
@@ -94,6 +100,13 @@ describe('Connection String', function () {
94100
expect(options.credentials.mechanism).to.equal(AuthMechanism.MONGODB_GSSAPI);
95101
});
96102

103+
it('should provide default authSource when valid AuthMechanism provided', function () {
104+
const options = parseOptions(
105+
'mongodb+srv://jira-sync.pw0q4.mongodb.net/testDB?authMechanism=MONGODB-AWS&retryWrites=true&w=majority'
106+
);
107+
expect(options.credentials.source).to.equal('$external');
108+
});
109+
97110
it('should parse a numeric authSource with variable width', function () {
98111
const options = parseOptions('mongodb://test@localhost/?authSource=0001');
99112
expect(options.credentials.source).to.equal('0001');
@@ -127,7 +140,7 @@ describe('Connection String', function () {
127140
});
128141

129142
it('does not set the ssl option', function () {
130-
expect(options.ssl).to.be.undefined;
143+
expect(options).to.not.have.property('ssl');
131144
});
132145
});
133146

@@ -139,7 +152,7 @@ describe('Connection String', function () {
139152
});
140153

141154
it('does not set the ssl option', function () {
142-
expect(options.ssl).to.be.undefined;
155+
expect(options).to.not.have.property('ssl');
143156
});
144157
});
145158
});
@@ -163,7 +176,7 @@ describe('Connection String', function () {
163176
});
164177

165178
it('does not set the ssl option', function () {
166-
expect(options.ssl).to.be.undefined;
179+
expect(options).to.not.have.property('ssl');
167180
});
168181
});
169182

@@ -176,7 +189,7 @@ describe('Connection String', function () {
176189
});
177190

178191
it('does not set the ssl option', function () {
179-
expect(options.ssl).to.be.undefined;
192+
expect(options).to.not.have.property('ssl');
180193
});
181194
});
182195

@@ -188,7 +201,7 @@ describe('Connection String', function () {
188201
});
189202

190203
it('does not set the ssl option', function () {
191-
expect(options.ssl).to.be.undefined;
204+
expect(options).to.not.have.property('ssl');
192205
});
193206
});
194207
});
@@ -312,4 +325,110 @@ describe('Connection String', function () {
312325
expect(options.srvHost).to.equal('test1.test.build.10gen.cc');
313326
});
314327
});
328+
329+
describe('resolveSRVRecord()', () => {
330+
const resolveSRVRecordAsync = promisify(resolveSRVRecord);
331+
332+
afterEach(async () => {
333+
sinon.restore();
334+
});
335+
336+
function makeStub(txtRecord: string) {
337+
const mockAddress = [
338+
{
339+
name: 'localhost.test.mock.test.build.10gen.cc',
340+
port: 2017,
341+
weight: 0,
342+
priority: 0
343+
}
344+
];
345+
346+
const mockRecord: string[][] = [[txtRecord]];
347+
348+
// first call is for stubbing resolveSrv
349+
// second call is for stubbing resolveTxt
350+
sinon.stub(dns, 'resolveSrv').callsFake((address, callback) => {
351+
return process.nextTick(callback, null, mockAddress);
352+
});
353+
354+
sinon.stub(dns, 'resolveTxt').callsFake((address, whatWeTest) => {
355+
whatWeTest(null, mockRecord);
356+
});
357+
}
358+
359+
for (const mechanism of AUTH_MECHS_AUTH_SRC_EXTERNAL) {
360+
it(`should set authSource to $external for ${mechanism} external mechanism`, async function () {
361+
makeStub('authSource=thisShouldNotBeAuthSource');
362+
const credentials = new MongoCredentials({
363+
source: '$external',
364+
mechanism,
365+
username: 'username',
366+
password: mechanism === AuthMechanism.MONGODB_X509 ? undefined : 'password',
367+
mechanismProperties: {}
368+
});
369+
credentials.validate();
370+
371+
const options = {
372+
credentials,
373+
srvHost: 'test.mock.test.build.10gen.cc',
374+
srvServiceName: 'mongodb',
375+
userSpecifiedAuthSource: false
376+
} as MongoOptions;
377+
378+
await resolveSRVRecordAsync(options);
379+
// check MongoCredentials instance (i.e. whether or not merge on options.credentials was called)
380+
expect(options).property('credentials').to.equal(credentials);
381+
expect(options).to.have.nested.property('credentials.source', '$external');
382+
});
383+
}
384+
385+
it('should set a default authSource for non-external mechanisms with no user-specified source', async function () {
386+
makeStub('authSource=thisShouldBeAuthSource');
387+
388+
const credentials = new MongoCredentials({
389+
source: 'admin',
390+
mechanism: AuthMechanism.MONGODB_SCRAM_SHA256,
391+
username: 'username',
392+
password: 'password',
393+
mechanismProperties: {}
394+
});
395+
credentials.validate();
396+
397+
const options = {
398+
credentials,
399+
srvHost: 'test.mock.test.build.10gen.cc',
400+
srvServiceName: 'mongodb',
401+
userSpecifiedAuthSource: false
402+
} as MongoOptions;
403+
404+
await resolveSRVRecordAsync(options);
405+
// check MongoCredentials instance (i.e. whether or not merge on options.credentials was called)
406+
expect(options).property('credentials').to.not.equal(credentials);
407+
expect(options).to.have.nested.property('credentials.source', 'thisShouldBeAuthSource');
408+
});
409+
410+
it('should retain credentials for any mechanism with no user-sepcificed source and no source in DNS', async function () {
411+
makeStub('');
412+
const credentials = new MongoCredentials({
413+
source: 'admin',
414+
mechanism: AuthMechanism.MONGODB_SCRAM_SHA256,
415+
username: 'username',
416+
password: 'password',
417+
mechanismProperties: {}
418+
});
419+
credentials.validate();
420+
421+
const options = {
422+
credentials,
423+
srvHost: 'test.mock.test.build.10gen.cc',
424+
srvServiceName: 'mongodb',
425+
userSpecifiedAuthSource: false
426+
} as MongoOptions;
427+
428+
await resolveSRVRecordAsync(options as any);
429+
// check MongoCredentials instance (i.e. whether or not merge on options.credentials was called)
430+
expect(options).property('credentials').to.equal(credentials);
431+
expect(options).to.have.nested.property('credentials.source', 'admin');
432+
});
433+
});
315434
});

0 commit comments

Comments
 (0)