From 8d18d50123648418acd1544f7592b5285eb4274f Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 26 Nov 2021 17:10:54 +0100 Subject: [PATCH 1/7] chore(data-service): Return authInfo as part of the instance info --- .../src/instance-detail-helper.spec.ts | 26 +++++++--- .../src/instance-detail-helper.ts | 49 ++++++++++++------- packages/instance-model/lib/model.js | 24 ++++++--- 3 files changed, 65 insertions(+), 34 deletions(-) diff --git a/packages/data-service/src/instance-detail-helper.spec.ts b/packages/data-service/src/instance-detail-helper.spec.ts index b9acba9097f..ed7a0361ce2 100644 --- a/packages/data-service/src/instance-detail-helper.spec.ts +++ b/packages/data-service/src/instance-detail-helper.spec.ts @@ -2,7 +2,7 @@ import { expect } from 'chai'; import { MongoClient } from 'mongodb'; import { ConnectionOptions } from './connection-options'; import { - extractPrivilegesByDatabaseAndCollection, + getPrivilegesByDatabaseAndCollection, getInstance, InstanceDetails, } from './instance-detail-helper'; @@ -42,6 +42,16 @@ describe('instance-detail-helper', function () { instanceDetails = await getInstance(mongoClient); }); + it('should have auth info', function () { + expect(instanceDetails).to.have.nested.property('auth.user', null); + expect(instanceDetails) + .to.have.nested.property('auth.roles') + .deep.eq([]); + expect(instanceDetails) + .to.have.nested.property('auth.privileges') + .deep.eq([]); + }); + it('should have build info', function () { expect(instanceDetails).to.have.nested.property( 'build.isEnterprise', @@ -246,7 +256,7 @@ describe('instance-detail-helper', function () { describe('#extractPrivilegesByDatabaseAndCollection', function () { it('returns a tree of databases and collections from privileges', function () { - const dbs = extractPrivilegesByDatabaseAndCollection([ + const dbs = getPrivilegesByDatabaseAndCollection([ { resource: { db: 'foo', collection: 'bar' }, actions: [] }, ]); @@ -255,7 +265,7 @@ describe('instance-detail-helper', function () { context('with known resources', function () { it('ignores cluster privileges', function () { - const dbs = extractPrivilegesByDatabaseAndCollection([ + const dbs = getPrivilegesByDatabaseAndCollection([ { resource: { db: 'foo', collection: 'bar' }, actions: [] }, { resource: { db: 'buz', collection: 'bla' }, actions: [] }, { resource: { cluster: true }, actions: [] }, @@ -272,7 +282,7 @@ describe('instance-detail-helper', function () { }); it('ignores anyResource privileges', function () { - const dbs = extractPrivilegesByDatabaseAndCollection([ + const dbs = getPrivilegesByDatabaseAndCollection([ { resource: { db: 'foo', collection: 'bar' }, actions: [] }, { resource: { db: 'buz', collection: 'bla' }, actions: [] }, { resource: { anyResource: true }, actions: [] }, @@ -291,7 +301,7 @@ describe('instance-detail-helper', function () { context('with unknown resources', function () { it("ignores everything that doesn't have database and collection in resource", function () { - const dbs = extractPrivilegesByDatabaseAndCollection([ + const dbs = getPrivilegesByDatabaseAndCollection([ { resource: { db: 'foo', collection: 'bar' }, actions: [] }, { resource: { db: 'buz', collection: 'bla' }, actions: [] }, { resource: { this: true }, actions: [] }, @@ -313,7 +323,7 @@ describe('instance-detail-helper', function () { }); it('keeps records for all collections in a database', function () { - const dbs = extractPrivilegesByDatabaseAndCollection([ + const dbs = getPrivilegesByDatabaseAndCollection([ { resource: { db: 'foo', collection: 'bar' }, actions: [] }, { resource: { db: 'foo', collection: 'buz' }, actions: [] }, { resource: { db: 'foo', collection: 'barbar' }, actions: [] }, @@ -322,14 +332,14 @@ describe('instance-detail-helper', function () { }); it('returns database actions indicated by empty collection name', function () { - const dbs = extractPrivilegesByDatabaseAndCollection([ + const dbs = getPrivilegesByDatabaseAndCollection([ { resource: { db: 'foo', collection: '' }, actions: ['find'] }, ]); expect(dbs.foo).to.have.property('').deep.eq(['find']); }); it('returns multiple databases and collections and their actions', function () { - const dbs = extractPrivilegesByDatabaseAndCollection([ + const dbs = getPrivilegesByDatabaseAndCollection([ { resource: { db: 'foo', collection: 'bar' }, actions: ['find'] }, { resource: { db: 'foo', collection: 'barbar' }, actions: ['find'] }, { resource: { db: 'buz', collection: 'foo' }, actions: ['insert'] }, diff --git a/packages/data-service/src/instance-detail-helper.ts b/packages/data-service/src/instance-detail-helper.ts index 3228aa30d66..4c32885895b 100644 --- a/packages/data-service/src/instance-detail-helper.ts +++ b/packages/data-service/src/instance-detail-helper.ts @@ -83,6 +83,11 @@ type DatabaseDetails = { }; export type InstanceDetails = { + auth: { + user: ConnectionStatusWithPrivileges['authInfo']['authenticatedUsers'][number] | null; + roles: ConnectionStatusWithPrivileges['authInfo']['authenticatedUserRoles']; + privileges: ConnectionStatusWithPrivileges['authInfo']['authenticatedUserPrivileges']; + }; build: BuildInfoDetails; host: HostInfoDetails; genuineMongoDB: GenuineMongoDBDetails; @@ -96,11 +101,16 @@ export async function getInstance( const adminDb = client.db('admin'); const [ + connectionStatus, getCmdLineOptsResult, hostInfoResult, buildInfoResult, getParameterResult, ] = await Promise.all([ + runCommand(adminDb, { connectionStatus: 1, showPrivileges: true }).catch( + ignoreNotAuthorized(null) + ), + runCommand(adminDb, { getCmdLineOpts: 1 }).catch((err) => { /** * This is something that mongodb-build-info uses to detect some @@ -125,6 +135,7 @@ export async function getInstance( ]); return { + auth: adaptAuthInfo(connectionStatus), build: adaptBuildInfo(buildInfoResult), host: adaptHostInfo(hostInfoResult), genuineMongoDB: buildGenuineMongoDBInfo( @@ -158,9 +169,27 @@ function buildDataLakeInfo(buildInfo: Partial): DataLakeDetails { }; } +function adaptAuthInfo(connectionStatus: ConnectionStatusWithPrivileges | null) { + if (connectionStatus === null) { + return { user: null, roles: [], privileges: [] }; + } + + const { + authenticatedUsers, + authenticatedUserRoles, + authenticatedUserPrivileges, + } = connectionStatus.authInfo; + + return { + user: authenticatedUsers[0] ?? null, + roles: authenticatedUserRoles, + privileges: authenticatedUserPrivileges, + }; +} + type DatabaseCollectionPrivileges = Record>; -export function extractPrivilegesByDatabaseAndCollection( +export function getPrivilegesByDatabaseAndCollection( authenticatedUserPrivileges: | ConnectionStatusWithPrivileges['authInfo']['authenticatedUserPrivileges'] | null = null, @@ -203,24 +232,6 @@ export function extractPrivilegesByDatabaseAndCollection( return result; } -type DatabasesAndCollectionsNames = Record>; - -export async function getDatabasesAndCollectionsFromPrivileges( - client: MongoClient, - requiredActions: string[] | null = null -): Promise { - const adminDb = client.db('admin'); - const connectionStatus = await runCommand(adminDb, { - connectionStatus: 1, - showPrivileges: true, - }).catch(ignoreNotAuthorized(null)); - - return extractPrivilegesByDatabaseAndCollection( - connectionStatus?.authInfo.authenticatedUserPrivileges, - requiredActions - ); -} - function isNotAuthorized(err: AnyError) { if (!err) { return false; diff --git a/packages/instance-model/lib/model.js b/packages/instance-model/lib/model.js index 84fbffbd3c9..f0a90ade551 100644 --- a/packages/instance-model/lib/model.js +++ b/packages/instance-model/lib/model.js @@ -52,6 +52,14 @@ function removeListenersRec(model) { } } +const AuthInfo = AmpersandModel.extend({ + props: { + user: { type: 'object', default: null }, + roles: { type: 'array', default: null }, + privileges: { type: 'array', default: null }, + }, +}); + const HostInfo = AmpersandModel.extend({ props: { arch: 'string', @@ -125,6 +133,7 @@ const InstanceModel = AmpersandModel.extend( build: BuildInfo, genuineMongoDB: GenuineMongoDB, dataLake: DataLake, + auth: AuthInfo, }, collections: { databases: MongoDbDatabaseCollection, @@ -180,13 +189,14 @@ const InstanceModel = AmpersandModel.extend( }); try { - // First fetch instance info and databases list, these are the essentials - // that we need to make Compass somewhat usable - await Promise.all([ - this.fetch({ dataService }), - shouldRefresh(this.databasesStatus, fetchDatabases) && - this.fetchDatabases({ dataService }), - ]); + // First fetch instance info ... + await this.fetch({ dataService }); + + // ... and databases list. These are the essentials that we need to make + // Compass somewhat usable + if (shouldRefresh(this.databasesStatus, fetchDatabases)) { + await this.fetchDatabases({ dataService }) + } // Then collection list for every database, namespace is the main thing // needed to be able to interact with any collection related tab From f7f3dc83bd28a7e5a8f18f1d4845113396e9abe7 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 26 Nov 2021 17:13:37 +0100 Subject: [PATCH 2/7] chore(data-service): Allow to provide privileges to avoid overfetching the authInfo --- .../data-service/src/data-service.spec.ts | 4 ++ packages/data-service/src/data-service.ts | 69 +++++++++++++++---- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/packages/data-service/src/data-service.spec.ts b/packages/data-service/src/data-service.spec.ts index b8ad43662f6..c94f602726b 100644 --- a/packages/data-service/src/data-service.spec.ts +++ b/packages/data-service/src/data-service.spec.ts @@ -1363,6 +1363,7 @@ describe('DataService', function () { listDatabases: { databases: [{ name: 'foo' }, { name: 'bar' }], }, + connectionStatus: { authInfo: { authenticatedUserPrivileges: [] } }, }, }); const dbs = (await dataService.listDatabases()).map((db) => db.name); @@ -1463,6 +1464,9 @@ describe('DataService', function () { describe('#listCollections', function () { it('returns collections for a database', async function () { const dataService = createDataServiceWithMockedClient({ + commands: { + connectionStatus: { authInfo: { authenticatedUserPrivileges: [] } }, + }, collections: { buz: ['foo', 'bar'], }, diff --git a/packages/data-service/src/data-service.ts b/packages/data-service/src/data-service.ts index 98baadf2641..0d5c3a2f7a1 100644 --- a/packages/data-service/src/data-service.ts +++ b/packages/data-service/src/data-service.ts @@ -55,7 +55,7 @@ import { ConnectionOptions } from './connection-options'; import { adaptCollectionInfo, adaptDatabaseInfo, - getDatabasesAndCollectionsFromPrivileges, + getPrivilegesByDatabaseAndCollection, getInstance, InstanceDetails, } from './instance-detail-helper'; @@ -69,7 +69,7 @@ import { } from './types'; import getPort from 'get-port'; -import { runCommand } from './run-command'; +import { ConnectionStatusWithPrivileges, runCommand } from './run-command'; // eslint-disable-next-line @typescript-eslint/no-var-requires const { fetch: getIndexes } = require('mongodb-index-model'); @@ -289,13 +289,40 @@ class DataService extends EventEmitter { return this._isMongos; } + async connectionStatus(): Promise { + const logop = this._startLogOp( + mongoLogId(1_001_000_100), + 'Running connectionStatus' + ); + try { + const adminDb = this._initializedClient.db('admin'); + const result = await runCommand(adminDb, { + connectionStatus: 1, + showPrivileges: true, + }); + logop(null); + return result; + } catch (e) { + logop(e); + throw e; + } + } + /** * List all collections for a database. */ async listCollections( databaseName: string, filter: Document = {}, - { nameOnly }: { nameOnly?: true } = {} + { + nameOnly, + privileges = null, + }: { + nameOnly?: true; + privileges?: + | ConnectionStatusWithPrivileges['authInfo']['authenticatedUserPrivileges'] + | null; + } = {} ): Promise[]> { const db = this._initializedClient.db(databaseName); const logop = this._startLogOp( @@ -326,9 +353,15 @@ class DataService extends EventEmitter { ); return [] as { name: string }[]; }), - getDatabasesAndCollectionsFromPrivileges(this._initializedClient, [ - 'find', - ]).then((databases) => { + (privileges + ? Promise.resolve(privileges) + : this.connectionStatus().then( + (status) => status.authInfo.authenticatedUserPrivileges + ) + ).then((privileges) => { + const databases = getPrivilegesByDatabaseAndCollection(privileges, [ + 'find', + ]); return Object.keys( // Privileges might not have a database we are looking for databases[databaseName] || {} @@ -364,8 +397,12 @@ class DataService extends EventEmitter { */ async listDatabases({ nameOnly, + privileges = null, }: { nameOnly?: true; + privileges?: + | ConnectionStatusWithPrivileges['authInfo']['authenticatedUserPrivileges'] + | null; } = {}): Promise[]> { const logop = this._startLogOp( mongoLogId(1_001_000_033), @@ -396,13 +433,19 @@ class DataService extends EventEmitter { ); return { databases: [] }; }), - // If we somehow failed to get user privileges to get a fallback for the - // databases/collections, we do want to hard fail, there is no good - // reason this command will ever fail, unless server is in a bad shape - // or we messed something up - getDatabasesAndCollectionsFromPrivileges(this._initializedClient, [ - 'find', - ]).then((databases) => { + (privileges + ? Promise.resolve(privileges) + : // If we somehow failed to get user privileges to get a fallback for the + // databases/collections, we do want to hard fail, there is no good + // reason this command will ever fail, unless server is in a bad shape + // or we messed something up + this.connectionStatus().then( + (status) => status.authInfo.authenticatedUserPrivileges + ) + ).then((privileges) => { + const databases = getPrivilegesByDatabaseAndCollection(privileges, [ + 'find', + ]); return { databases: Object.keys(databases) .filter( From 1df12b15af0e6a215662cb4e48aedd890240c691 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 26 Nov 2021 17:14:36 +0100 Subject: [PATCH 3/7] chore(database-model, collection-model): Pass instance privileges when fetching db/coll list to avoid overfetching authInfo --- packages/collection-model/lib/model.js | 23 ++++++++++++++++++++--- packages/database-model/lib/model.js | 21 ++++++++++++++++++++- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/packages/collection-model/lib/model.js b/packages/collection-model/lib/model.js index c27a3165023..5216414d324 100644 --- a/packages/collection-model/lib/model.js +++ b/packages/collection-model/lib/model.js @@ -83,6 +83,15 @@ function propagateCollectionEvents(namespace) { }; } +function getParentByType(model, type) { + const parent = getParent(model); + return parent + ? parent.modelType === type + ? parent + : getParentByType(parent, type) + : null; +} + function pickCollectionInfo({ type, readonly, @@ -235,18 +244,26 @@ const CollectionCollection = AmpersandCollection.extend( * @returns {Promise} */ async fetch({ dataService, fetchInfo = true }) { - const databaseName = this.parent && this.parent.getId(); + const databaseName = getParentByType(this, 'Database')?.getId(); if (!databaseName) { throw new Error( - "Trying to fetch MongoDBCollectionCollection that doesn't have the parent model" + `Trying to fetch ${this.modelType} that doesn't have the Database parent model` + ); + } + + const instanceModel = getParentByType(this, 'Instance'); + + if (!instanceModel) { + throw new Error( + `Trying to fetch ${this.modelType} that doesn't have the Instance parent model` ); } const collections = await dataService.listCollections( databaseName, {}, - { nameOnly: !fetchInfo } + { nameOnly: !fetchInfo, privileges: instanceModel.auth.privileges } ); this.set( diff --git a/packages/database-model/lib/model.js b/packages/database-model/lib/model.js index 32f68522099..0ae55098fd5 100644 --- a/packages/database-model/lib/model.js +++ b/packages/database-model/lib/model.js @@ -75,6 +75,15 @@ function propagateCollectionEvents(namespace) { }; } +function getParentByType(model, type) { + const parent = getParent(model); + return parent + ? parent.modelType === type + ? parent + : getParentByType(parent, type) + : null; +} + const DatabaseModel = AmpersandModel.extend( debounceActions(['fetch', 'fetchCollections']), { @@ -169,7 +178,17 @@ const DatabaseCollection = AmpersandCollection.extend( * @returns {Promise} */ async fetch({ dataService }) { - const dbs = await dataService.listDatabases({ nameOnly: true }); + const instanceModel = getParentByType(this, 'Instance'); + if (!instanceModel) { + throw new Error( + `Trying to fetch ${this.modelType} that doesn't have the Instance parent model` + ); + } + + const dbs = await dataService.listDatabases({ + nameOnly: true, + privileges: instanceModel.auth.privileges, + }); this.set(dbs.map(({ _id, name }) => ({ _id, name }))); }, From 7dc965d983760703b78ecf896c1f92fdc636a2ca Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 26 Nov 2021 17:28:13 +0100 Subject: [PATCH 4/7] fix(sidebar, databases-collections): Avoid too many state updates when global overlay is active --- packages/compass-sidebar/src/stores/store.js | 47 ++++++++------- .../src/stores/collections-store.js | 60 +++++++++++-------- .../src/stores/databases-store.js | 24 +++++--- 3 files changed, 77 insertions(+), 54 deletions(-) diff --git a/packages/compass-sidebar/src/stores/store.js b/packages/compass-sidebar/src/stores/store.js index 7adff4bb01e..0d32f6460d4 100644 --- a/packages/compass-sidebar/src/stores/store.js +++ b/packages/compass-sidebar/src/stores/store.js @@ -43,26 +43,33 @@ store.onActivated = (appRegistry) => { onInstanceChange(instance); onDatabasesChange(instance.databases); - instance.on('change:isRefreshing', () => { - onInstanceChange(instance); - }); - - instance.on('change:status', () => { - onInstanceChange(instance); - }); - - instance.on('change:databasesStatus', () => { - onInstanceChange(instance); - onDatabasesChange(instance.databases); - }); - - instance.on('change:databases.collectionsLength', () => { - onInstanceChange(instance); - }); - - instance.on('change:databases.collectionsStatus', () => { - onDatabasesChange(instance.databases); - }); + if (process.env.COMPASS_NO_GLOBAL_OVERLAY !== 'true') { + instance.on('change:isRefreshing', () => { + onInstanceChange(instance); + onDatabasesChange(instance.databases); + }); + } else { + instance.on('change:isRefreshing', () => { + onInstanceChange(instance); + }); + + instance.on('change:status', () => { + onInstanceChange(instance); + }); + + instance.on('change:databases.collectionsLength', () => { + onInstanceChange(instance); + }); + + instance.on('change:databasesStatus', () => { + onInstanceChange(instance); + onDatabasesChange(instance.databases); + }); + + instance.on('change:databases.collectionsStatus', () => { + onDatabasesChange(instance.databases); + }); + } function onIsGenuineChange(isGenuine) { store.dispatch(toggleIsGenuineMongoDB(!!isGenuine)); diff --git a/packages/databases-collections/src/stores/collections-store.js b/packages/databases-collections/src/stores/collections-store.js index e5874bcb829..31722c91922 100644 --- a/packages/databases-collections/src/stores/collections-store.js +++ b/packages/databases-collections/src/stores/collections-store.js @@ -13,18 +13,25 @@ import { collectionsReducer } from '../modules'; const store = createStore(collectionsReducer, applyMiddleware(thunk)); store.onActivated = (appRegistry) => { - const onCollectionsChange = throttle( - (collections, databases, dbName = store.getState().databaseName) => { - collections = collections ?? databases.get(dbName)?.collections; - if (collections && collections.parent.getId() === dbName) { + const onCollectionsChange = throttle((collections, force = false) => { + const { databaseName } = store.getState(); + if (collections.parent.getId() === databaseName) { + if (process.env.COMPASS_NO_GLOBAL_OVERLAY !== 'true') { + const shouldUpdate = force || !collections.some((coll) => + ['fetching', 'refreshing'].includes(coll.status) + ); + if (shouldUpdate) { + store.dispatch(loadCollections(collections.toJSON())); + } + } else { store.dispatch(loadCollections(collections.toJSON())); } - }, - 100 - ); + } + }, 100); appRegistry.on('instance-destroyed', () => { onCollectionsChange.cancel(); + store.instance = null; }); /** @@ -33,14 +40,10 @@ store.onActivated = (appRegistry) => { * @param {Object} state - The instance store state. */ appRegistry.on('instance-created', ({ instance }) => { - onCollectionsChange(null, instance.databases); - - instance.dataLake.on('change:isDataLake', (model, isDataLake) => { - store.dispatch(toggleIsDataLake(isDataLake)); - }); + store.instance = instance; instance.on('change:databases.collectionsStatus', (model) => { - onCollectionsChange(model.collections, instance.databases); + onCollectionsChange(model.collections); }); instance.on('change:collections.status', (model) => { @@ -48,23 +51,30 @@ store.onActivated = (appRegistry) => { // collection that holds references to all collection models on the // database. Above `collections` is a reference the collections property // on the database model - onCollectionsChange(model.collection, instance.databases); + onCollectionsChange(model.collection); }); - /** - * When the database changes load the collections. - * - * @param {String} ns - The namespace. - */ - appRegistry.on('select-database', (ns) => { - const { databaseName } = store.getState(); - if (ns && !ns.includes('.') && ns !== databaseName) { - store.dispatch(changeDatabaseName(ns)); - onCollectionsChange(null, instance.databases, ns); - } + instance.dataLake.on('change:isDataLake', (model, isDataLake) => { + store.dispatch(toggleIsDataLake(isDataLake)); }); }); + /** + * When the database changes load the collections. + * + * @param {String} ns - The namespace. + */ + appRegistry.on('select-database', (ns) => { + const { databaseName } = store.getState(); + if (ns !== databaseName) { + store.dispatch(changeDatabaseName(ns)); + onCollectionsChange( + store.instance?.databases.get(ns)?.collections ?? [], + true + ); + } + }); + /** * When write state changes based on SDAM events we change the store state. * diff --git a/packages/databases-collections/src/stores/databases-store.js b/packages/databases-collections/src/stores/databases-store.js index 6e78149e653..17e7d392fd8 100644 --- a/packages/databases-collections/src/stores/databases-store.js +++ b/packages/databases-collections/src/stores/databases-store.js @@ -36,17 +36,23 @@ store.onActivated = (appRegistry) => { store.dispatch(toggleIsDataLake(newVal)); }); - instance.on('change:databasesStatus', () => { - onDatabasesChange(instance.databases); - }); + if (process.env.COMPASS_NO_GLOBAL_OVERLAY !== 'true') { + instance.on('change:isRefreshing', () => { + onDatabasesChange(instance.databases); + }); + } else { + instance.on('change:databasesStatus', () => { + onDatabasesChange(instance.databases); + }); - instance.on('change:databases.status', () => { - onDatabasesChange(instance.databases); - }); + instance.on('change:databases.status', () => { + onDatabasesChange(instance.databases); + }); - instance.on('change:databases.collectionsLength', () => { - onDatabasesChange(instance.databases); - }); + instance.on('change:databases.collectionsLength', () => { + onDatabasesChange(instance.databases); + }); + } }); /** From 92a3ddd8b7284284228471e330230f82173ab496 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 26 Nov 2021 18:23:44 +0100 Subject: [PATCH 5/7] chore: Prettier --- packages/data-service/src/instance-detail-helper.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/data-service/src/instance-detail-helper.ts b/packages/data-service/src/instance-detail-helper.ts index 4c32885895b..e78e1eda518 100644 --- a/packages/data-service/src/instance-detail-helper.ts +++ b/packages/data-service/src/instance-detail-helper.ts @@ -84,7 +84,9 @@ type DatabaseDetails = { export type InstanceDetails = { auth: { - user: ConnectionStatusWithPrivileges['authInfo']['authenticatedUsers'][number] | null; + user: + | ConnectionStatusWithPrivileges['authInfo']['authenticatedUsers'][number] + | null; roles: ConnectionStatusWithPrivileges['authInfo']['authenticatedUserRoles']; privileges: ConnectionStatusWithPrivileges['authInfo']['authenticatedUserPrivileges']; }; @@ -169,7 +171,9 @@ function buildDataLakeInfo(buildInfo: Partial): DataLakeDetails { }; } -function adaptAuthInfo(connectionStatus: ConnectionStatusWithPrivileges | null) { +function adaptAuthInfo( + connectionStatus: ConnectionStatusWithPrivileges | null +) { if (connectionStatus === null) { return { user: null, roles: [], privileges: [] }; } From b791155bdeb6c141e31437c4c6755133391a922c Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Mon, 29 Nov 2021 09:58:33 +0100 Subject: [PATCH 6/7] chore(data-service): Move privileges fallback logic to its own private method --- packages/data-service/src/data-service.ts | 32 +++++++++++------------ 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/data-service/src/data-service.ts b/packages/data-service/src/data-service.ts index 0d5c3a2f7a1..27c1cfa2caa 100644 --- a/packages/data-service/src/data-service.ts +++ b/packages/data-service/src/data-service.ts @@ -308,6 +308,20 @@ class DataService extends EventEmitter { } } + private async _getPrivilegesOrFallback( + privileges: + | ConnectionStatusWithPrivileges['authInfo']['authenticatedUserPrivileges'] + | null = null + ) { + if (privileges) { + return privileges; + } + const { + authInfo: { authenticatedUserPrivileges }, + } = await this.connectionStatus(); + return authenticatedUserPrivileges; + } + /** * List all collections for a database. */ @@ -353,12 +367,7 @@ class DataService extends EventEmitter { ); return [] as { name: string }[]; }), - (privileges - ? Promise.resolve(privileges) - : this.connectionStatus().then( - (status) => status.authInfo.authenticatedUserPrivileges - ) - ).then((privileges) => { + this._getPrivilegesOrFallback(privileges).then((privileges) => { const databases = getPrivilegesByDatabaseAndCollection(privileges, [ 'find', ]); @@ -433,16 +442,7 @@ class DataService extends EventEmitter { ); return { databases: [] }; }), - (privileges - ? Promise.resolve(privileges) - : // If we somehow failed to get user privileges to get a fallback for the - // databases/collections, we do want to hard fail, there is no good - // reason this command will ever fail, unless server is in a bad shape - // or we messed something up - this.connectionStatus().then( - (status) => status.authInfo.authenticatedUserPrivileges - ) - ).then((privileges) => { + this._getPrivilegesOrFallback(privileges).then((privileges) => { const databases = getPrivilegesByDatabaseAndCollection(privileges, [ 'find', ]); From 956d7666cd1c3da19911530240477c58160e6ce4 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Mon, 29 Nov 2021 10:15:00 +0100 Subject: [PATCH 7/7] chore(data-service): Move all Promise.all logic for list methods to scoped functions --- packages/data-service/src/data-service.ts | 167 ++++++++++++---------- 1 file changed, 93 insertions(+), 74 deletions(-) diff --git a/packages/data-service/src/data-service.ts b/packages/data-service/src/data-service.ts index 27c1cfa2caa..31c1333331e 100644 --- a/packages/data-service/src/data-service.ts +++ b/packages/data-service/src/data-service.ts @@ -338,51 +338,59 @@ class DataService extends EventEmitter { | null; } = {} ): Promise[]> { - const db = this._initializedClient.db(databaseName); const logop = this._startLogOp( mongoLogId(1_001_000_032), 'Running listCollections', { db: databaseName, nameOnly: nameOnly ?? false } ); + + const db = this._initializedClient.db(databaseName); + + const listCollections = async () => { + try { + return db.listCollections(filter, { nameOnly }).toArray(); + } catch (err) { + // Currently Compass should not fail if listCollections failed for + // any possible reason to preserve current behavior. We probably + // want this to check at least that what we got back is a server + // error and not a weird runtime issue on our side that can be + // swallowed in this case, ideally we know exactly what server + // errors we want to handle here and only avoid throwing in these + // cases + // + // TODO: https://jira.mongodb.org/browse/COMPASS-5275 + log.warn( + mongoLogId(1_001_000_099), + this._logCtx(), + 'Failed to run listCollections', + { message: (err as Error).message } + ); + return [] as { name: string }[]; + } + }; + + const getCollectionsFromPrivileges = async () => { + const databases = getPrivilegesByDatabaseAndCollection( + await this._getPrivilegesOrFallback(privileges), + ['find'] + ); + return Object.keys( + // Privileges might not have a database we are looking for + databases[databaseName] || {} + ) + .filter( + // Privileges can have collection name '' that indicates + // privileges on all collections in the database, we don't want + // those registered as "real" collection names + Boolean + ) + .map((name) => ({ name })); + }; + try { const [listedCollections, collectionsFromPrivileges] = await Promise.all([ - db - .listCollections(filter, { nameOnly }) - .toArray() - .catch((err) => { - // Currently Compass should not fail if listCollections failed for - // any possible reason to preserve current behavior. We probably - // want this to check at least that what we got back is a server - // error and not a weird runtime issue on our side that can be - // swallowed in this case, ideally we know exactly what server - // errors we want to handle here and only avoid throwing in these - // cases - // - // TODO: https://jira.mongodb.org/browse/COMPASS-5275 - log.warn( - mongoLogId(1_001_000_099), - this._logCtx(), - 'Failed to run listCollections', - { message: err.message } - ); - return [] as { name: string }[]; - }), - this._getPrivilegesOrFallback(privileges).then((privileges) => { - const databases = getPrivilegesByDatabaseAndCollection(privileges, [ - 'find', - ]); - return Object.keys( - // Privileges might not have a database we are looking for - databases[databaseName] || {} - ) - .filter( - // Privileges can have collection name '' that indicates - // privileges on all collections in the database, we don't want - // those registered as "real" collection names - Boolean - ) - .map((name) => ({ name })); - }), + listCollections(), + getCollectionsFromPrivileges(), ]); const collections = uniqueBy( @@ -421,49 +429,60 @@ class DataService extends EventEmitter { const adminDb = this._initializedClient.db('admin'); + const listDatabases = async () => { + try { + const { databases } = await runCommand(adminDb, { + listDatabases: 1, + nameOnly, + } as { + listDatabases: 1; + }); + return databases; + } catch (err) { + // Currently Compass should not fail if listDatabase failed for any + // possible reason to preserve current behavior. We probably want this + // to check at least that what we got back is a server error and not a + // weird runtime issue on our side that can be swallowed in this case, + // ideally we know exactly what server errors we want to handle here + // and only avoid throwing in these cases + // + // TODO: https://jira.mongodb.org/browse/COMPASS-5275 + log.warn( + mongoLogId(1_001_000_098), + this._logCtx(), + 'Failed to run listDatabases', + { message: (err as Error).message } + ); + return []; + } + }; + + const getDatabasesFromPrivileges = async () => { + const databases = getPrivilegesByDatabaseAndCollection( + await this._getPrivilegesOrFallback(privileges), + ['find'] + ); + return Object.keys(databases) + .filter( + // For the roles created in admin database, the database name + // can be '' meaning that it applies to all databases. We can't + // meaningfully handle this in the UI so we are filtering these + // out + Boolean + ) + .map((name) => ({ name })); + }; + try { const [listedDatabases, databasesFromPrivileges] = await Promise.all([ - runCommand(adminDb, { listDatabases: 1, nameOnly } as { - listDatabases: 1; - }).catch((err) => { - // Currently Compass should not fail if listDatabase failed for any - // possible reason to preserve current behavior. We probably want this - // to check at least that what we got back is a server error and not a - // weird runtime issue on our side that can be swallowed in this case, - // ideally we know exactly what server errors we want to handle here - // and only avoid throwing in these cases - // - // TODO: https://jira.mongodb.org/browse/COMPASS-5275 - log.warn( - mongoLogId(1_001_000_098), - this._logCtx(), - 'Failed to run listDatabases', - { message: err.message } - ); - return { databases: [] }; - }), - this._getPrivilegesOrFallback(privileges).then((privileges) => { - const databases = getPrivilegesByDatabaseAndCollection(privileges, [ - 'find', - ]); - return { - databases: Object.keys(databases) - .filter( - // For the roles created in admin database, the database name - // can be '' meaning that it applies to all databases. We can't - // meaningfully handle this in the UI so we are filtering these - // out - Boolean - ) - .map((name) => ({ name })), - }; - }), + listDatabases(), + getDatabasesFromPrivileges(), ]); const databases = uniqueBy( // NB: Order is important, we want listed collections to take precedence // if they were fetched successfully - [...databasesFromPrivileges.databases, ...listedDatabases.databases], + [...databasesFromPrivileges, ...listedDatabases], 'name' ).map((db) => adaptDatabaseInfo({ db: db.name, ...db }));