From 89f2f808edd1efe41ce25301fdd8baa4b016ca37 Mon Sep 17 00:00:00 2001 From: Sam Zhang Date: Wed, 5 Jan 2022 14:53:00 -0500 Subject: [PATCH 1/9] update mock authSource --- test/unit/connection_string.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 171284ad4e..690ad763e6 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -334,5 +334,20 @@ describe('Connection String', function () { expect(options).property('credentials').to.equal(credentials); expect(options).to.have.nested.property('credentials.source', 'admin'); }); + + it('should retain specified authSource with no provided credentials', async function () { + makeStub('authSource=thisShouldBeAuthSource'); + const credentials = {}; + + const options = { + credentials, + srvHost: 'test.mock.test.build.10gen.cc', + srvServiceName: 'mongodb', + userSpecifiedAuthSource: false + } as MongoOptions; + + await resolveSRVRecordAsync(options as any); + expect(options).to.have.nested.property('credentials.source', 'thisShouldBeAuthSource'); + }); }); }); From 70d6d8984200f52e4422234c0666f90bc4e950f9 Mon Sep 17 00:00:00 2001 From: Sam Zhang Date: Wed, 5 Jan 2022 14:54:50 -0500 Subject: [PATCH 2/9] format --- test/unit/connection_string.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 690ad763e6..fc25bac062 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -338,7 +338,6 @@ describe('Connection String', function () { it('should retain specified authSource with no provided credentials', async function () { makeStub('authSource=thisShouldBeAuthSource'); const credentials = {}; - const options = { credentials, srvHost: 'test.mock.test.build.10gen.cc', From 4bff0e94f025916c44b7dc04e240c44b8ebee0f1 Mon Sep 17 00:00:00 2001 From: Sam Zhang Date: Thu, 6 Jan 2022 11:37:19 -0500 Subject: [PATCH 3/9] add test case for non-srv URI --- test/unit/connection_string.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index fc25bac062..d1932325f8 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -89,6 +89,15 @@ describe('Connection String', function () { expect(options.credentials.source).to.equal('$external'); }); + it('should retain user-specified authSource with no credentials for non-srv URI', function () { + const mockAuthSource = 'thisShouldBeAuthSource'; + const options = parseOptions(`mongodb://localhost/?authSource=${mockAuthSource}`); + + expect(options).to.have.nested.property('credentials.username', ''); + expect(options).to.have.nested.property('credentials.mechanism', 'DEFAULT'); + expect(options).to.have.nested.property('credentials.source', mockAuthSource); + }); + it('should parse a numeric authSource with variable width', function () { const options = parseOptions('mongodb://test@localhost/?authSource=0001'); expect(options.credentials.source).to.equal('0001'); @@ -346,6 +355,8 @@ describe('Connection String', function () { } as MongoOptions; await resolveSRVRecordAsync(options as any); + expect(options).to.have.nested.property('credentials.username', ''); + expect(options).to.have.nested.property('credentials.mechanism', 'DEFAULT'); expect(options).to.have.nested.property('credentials.source', 'thisShouldBeAuthSource'); }); }); From dbaf782250ec263a33669013b9d87924f33f1e78 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 19 Jan 2022 11:19:50 -0500 Subject: [PATCH 4/9] fix: Add test and logic to ignore credentials if authSource is the only auth option --- src/connection_string.ts | 10 ++++++++++ test/unit/connection_string.test.ts | 27 +++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 079dfd3f51..54a9ab3fed 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -394,6 +394,16 @@ export function parseOptions( } mongoOptions.credentials.validate(); + + // Check if the only auth related option provided was authSource, if so we can remove credentials + if ( + mongoOptions.credentials.password === '' && + mongoOptions.credentials.username === '' && + mongoOptions.credentials.mechanism === AuthMechanism.MONGODB_DEFAULT && + Object.keys(mongoOptions.credentials.mechanismProperties).length === 0 + ) { + delete mongoOptions.credentials; + } } if (!mongoOptions.dbName) { diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index d1932325f8..0176268a6d 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -6,8 +6,13 @@ import { promisify } from 'util'; import { MongoCredentials } from '../../src/cmap/auth/mongo_credentials'; import { AUTH_MECHS_AUTH_SRC_EXTERNAL, AuthMechanism } from '../../src/cmap/auth/providers'; import { parseOptions, resolveSRVRecord } from '../../src/connection_string'; -import { MongoDriverError, MongoInvalidArgumentError, MongoParseError } from '../../src/error'; -import { MongoOptions } from '../../src/mongo_client'; +import { + MongoDriverError, + MongoInvalidArgumentError, + MongoParseError, + MongoServerSelectionError +} from '../../src/error'; +import { MongoClient, MongoOptions } from '../../src/mongo_client'; describe('Connection String', function () { it('should not support auth passed with user', function () { @@ -98,6 +103,24 @@ describe('Connection String', function () { expect(options).to.have.nested.property('credentials.source', mockAuthSource); }); + it('should omit credentials if the only auth related option is authSource', async () => { + const client = new MongoClient('mongodb://localhost:123/?authSource=someDb', { + serverSelectionTimeoutMS: 500 + }); + + let thrownError: Error; + try { + // relies on us not running a mongod on port 123, fairly likely assumption + await client.connect(); + } catch (error) { + thrownError = error; + } + + // We should fail to connect, not fail to find an auth provider + expect(thrownError).to.be.instanceOf(MongoServerSelectionError); + expect(client.options).to.not.have.a.property('credentials'); + }); + it('should parse a numeric authSource with variable width', function () { const options = parseOptions('mongodb://test@localhost/?authSource=0001'); expect(options.credentials.source).to.equal('0001'); From d51616f5b47d9c9a42e8ab989840335c8bd87790 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 19 Jan 2022 15:17:56 -0500 Subject: [PATCH 5/9] chore: add reasoning for test structure --- test/unit/connection_string.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 0176268a6d..2f9e2c1a92 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -104,6 +104,12 @@ describe('Connection String', function () { }); it('should omit credentials if the only auth related option is authSource', async () => { + // The error we're looking to **not** see is + // `new MongoInvalidArgumentError('No AuthProvider for ${credentials.mechanism} defined.')` + // in `prepareHandshakeDocument` and/or `performInitialHandshake`. + // Neither function is exported currently but if I did export them because of the inlined callbacks + // I think I would need to mock quite a bit of internals to get down to that layer. + // My thinking is I can lean on server selection failing for th unit tests to assert we at least don't get an error related to auth. const client = new MongoClient('mongodb://localhost:123/?authSource=someDb', { serverSelectionTimeoutMS: 500 }); From 1d76d1529c22a2db398057a68dd18e98bea87c84 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 19 Jan 2022 15:42:07 -0500 Subject: [PATCH 6/9] fix: update assertion, and test name --- test/unit/connection_string.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 2f9e2c1a92..43762d799d 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -7,10 +7,10 @@ import { MongoCredentials } from '../../src/cmap/auth/mongo_credentials'; import { AUTH_MECHS_AUTH_SRC_EXTERNAL, AuthMechanism } from '../../src/cmap/auth/providers'; import { parseOptions, resolveSRVRecord } from '../../src/connection_string'; import { + MongoAPIError, MongoDriverError, MongoInvalidArgumentError, - MongoParseError, - MongoServerSelectionError + MongoParseError } from '../../src/error'; import { MongoClient, MongoOptions } from '../../src/mongo_client'; @@ -103,7 +103,7 @@ describe('Connection String', function () { expect(options).to.have.nested.property('credentials.source', mockAuthSource); }); - it('should omit credentials if the only auth related option is authSource', async () => { + it('should omit credentials and throw a non-MongoAPIError if the only auth related option is authSource', async () => { // The error we're looking to **not** see is // `new MongoInvalidArgumentError('No AuthProvider for ${credentials.mechanism} defined.')` // in `prepareHandshakeDocument` and/or `performInitialHandshake`. @@ -122,8 +122,8 @@ describe('Connection String', function () { thrownError = error; } - // We should fail to connect, not fail to find an auth provider - expect(thrownError).to.be.instanceOf(MongoServerSelectionError); + // We should fail to connect, not fail to find an auth provider thus we should not find a MongoAPIError + expect(thrownError).to.not.be.instanceOf(MongoAPIError); expect(client.options).to.not.have.a.property('credentials'); }); From 5392813d45e9a913871da23b71f18e82ca372bbe Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 19 Jan 2022 16:06:44 -0500 Subject: [PATCH 7/9] test: title phrasing Co-authored-by: Daria Pardue --- test/unit/connection_string.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 43762d799d..3332814876 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -103,7 +103,7 @@ describe('Connection String', function () { expect(options).to.have.nested.property('credentials.source', mockAuthSource); }); - it('should omit credentials and throw a non-MongoAPIError if the only auth related option is authSource', async () => { + it('should omit credentials and not throw a MongoAPIError if the only auth related option is authSource', async () => { // The error we're looking to **not** see is // `new MongoInvalidArgumentError('No AuthProvider for ${credentials.mechanism} defined.')` // in `prepareHandshakeDocument` and/or `performInitialHandshake`. From d32dd00f9b2ae8a38107ff06133e2b1d83b04e55 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 20 Jan 2022 11:13:36 -0500 Subject: [PATCH 8/9] fix: failing test --- test/unit/connection_string.test.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 43762d799d..65682f3b79 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -94,13 +94,11 @@ describe('Connection String', function () { expect(options.credentials.source).to.equal('$external'); }); - it('should retain user-specified authSource with no credentials for non-srv URI', function () { - const mockAuthSource = 'thisShouldBeAuthSource'; - const options = parseOptions(`mongodb://localhost/?authSource=${mockAuthSource}`); - - expect(options).to.have.nested.property('credentials.username', ''); - expect(options).to.have.nested.property('credentials.mechanism', 'DEFAULT'); - expect(options).to.have.nested.property('credentials.source', mockAuthSource); + it('should retain user-specified authSource with no credentials', function () { + let options = parseOptions(`mongodb://a/?authSource=someDb`); + expect(options).to.not.have.property('credentials'); + options = parseOptions(`mongodb+srv://a/?authSource=someDb`); + expect(options).to.not.have.property('credentials'); }); it('should omit credentials and throw a non-MongoAPIError if the only auth related option is authSource', async () => { From fae3e65d71e53a8814d3bd27e482d202bfb4ab11 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 21 Jan 2022 10:57:11 -0500 Subject: [PATCH 9/9] fix: test name --- test/unit/connection_string.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 7520acd296..c051a95221 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -94,7 +94,7 @@ describe('Connection String', function () { expect(options.credentials.source).to.equal('$external'); }); - it('should retain user-specified authSource with no credentials', function () { + it('should omit credentials option when the only authSource is provided', function () { let options = parseOptions(`mongodb://a/?authSource=someDb`); expect(options).to.not.have.property('credentials'); options = parseOptions(`mongodb+srv://a/?authSource=someDb`);