Skip to content

fix(data-service): use databases where user has built in roles in listDatabases COMPASS-5691 #2959

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 7, 2022
Merged

fix(data-service): use databases where user has built in roles in listDatabases COMPASS-5691 #2959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
614141b
feat: use databases where the authenticated user has built in roles f…
Anemy Apr 6, 2022
58d81b0
fixup: minimize diff, remove extra newline
Anemy Apr 6, 2022
a4706d1
Merge branch 'main' into COMPASS-5691-use-built-in-roles-for-displayi…
lerouxb Apr 7, 2022
67838e5
Merge branch 'main' into COMPASS-5691-use-built-in-roles-for-displayi…
Anemy Apr 7, 2022
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
68 changes: 66 additions & 2 deletions packages/data-service/src/data-service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1396,6 +1396,56 @@ describe('DataService', function () {
expect(dbs).to.deep.eq(['foo']);
});

it('returns databases with `read`, `readWrite`, `dbAdmin`, `dbOwner` roles roles', async function () {
const dataService = createDataServiceWithMockedClient({
commands: {
connectionStatus: {
authInfo: {
authenticatedUserPrivileges: [],
authenticatedUserRoles: [
{
role: 'readWrite',
db: 'pineapple',
},
{
role: 'dbAdmin',
db: 'pineapple',
},
{
role: 'dbAdmin',
db: 'readerOfPineapple',
},
{
role: 'dbOwner',
db: 'pineappleBoss',
},
{
role: 'customRole',
db: 'mint',
},
{
role: 'read',
db: 'foo',
},
{
role: 'readWrite',
db: 'watermelon',
},
],
},
},
},
});
const dbs = (await dataService.listDatabases()).map((db) => db.name);
expect(dbs).to.deep.eq([
'pineapple',
'readerOfPineapple',
'pineappleBoss',
'foo',
'watermelon',
]);
});

it('filters out databases with no name from privileges', async function () {
const dataService = createDataServiceWithMockedClient({
commands: {
Expand All @@ -1419,7 +1469,7 @@ describe('DataService', function () {
expect(dbs).to.deep.eq(['bar']);
});

it('merges databases from listDatabases and privileges', async function () {
it('merges databases from listDatabases, privileges, and roles', async function () {
const dataService = createDataServiceWithMockedClient({
commands: {
listDatabases: { databases: [{ name: 'foo' }, { name: 'bar' }] },
Expand All @@ -1435,12 +1485,26 @@ describe('DataService', function () {
actions: ['find'],
},
],
authenticatedUserRoles: [
{
role: 'readWrite',
db: 'pineapple',
},
{
role: 'dbAdmin',
db: 'pineapple',
},
{
role: 'customRole',
db: 'mint',
},
],
},
},
},
});
const dbs = (await dataService.listDatabases()).map((db) => db.name);
expect(dbs).to.deep.eq(['foo', 'buz', 'bar']);
expect(dbs).to.deep.eq(['pineapple', 'foo', 'buz', 'bar']);
});

it('returns result from privileges even if listDatabases threw any error', async function () {
Expand Down
47 changes: 42 additions & 5 deletions packages/data-service/src/data-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import {
getPrivilegesByDatabaseAndCollection,
checkIsCSFLEConnection,
getInstance,
getDatabasesByRoles,
} from './instance-detail-helper';
import { redactConnectionString } from './redact';
import connectMongoClient from './connect-mongo-client';
Expand Down Expand Up @@ -344,6 +345,21 @@ class DataService extends EventEmitter {
return authenticatedUserPrivileges;
}

private async _getRolesOrFallback(
roles:
| ConnectionStatusWithPrivileges['authInfo']['authenticatedUserRoles']
| null = null
) {
if (roles) {
return roles;
}
const {
authInfo: { authenticatedUserRoles },
} = await this.connectionStatus();

return authenticatedUserRoles;
}

/**
* List all collections for a database.
*/
Expand Down Expand Up @@ -442,11 +458,15 @@ class DataService extends EventEmitter {
async listDatabases({
nameOnly,
privileges = null,
roles = null,
}: {
nameOnly?: true;
privileges?:
| ConnectionStatusWithPrivileges['authInfo']['authenticatedUserPrivileges']
| null;
roles?:
| ConnectionStatusWithPrivileges['authInfo']['authenticatedUserRoles']
| null;
} = {}): Promise<{ _id: string; name: string }[]> {
const logop = this._startLogOp(
mongoLogId(1_001_000_033),
Expand Down Expand Up @@ -500,16 +520,33 @@ class DataService extends EventEmitter {
.map((name) => ({ name }));
};

const getDatabasesFromRoles = async () => {
const databases = getDatabasesByRoles(
await this._getRolesOrFallback(roles),
// https://jira.mongodb.org/browse/HELP-32199
// Atlas shared tier MongoDB server version v5+ does not return
// `authenticatedUserPrivileges` as part of the `connectionStatus`.
// As a workaround we show the databases the user has
// certain general built-in roles for.
// This does not cover custom user roles which can
// have custom privileges that we can't currently fetch.
['read', 'readWrite', 'dbAdmin', 'dbOwner']
);
return databases.map((name) => ({ name }));
};

try {
const [listedDatabases, databasesFromPrivileges] = await Promise.all([
listDatabases(),
getDatabasesFromPrivileges(),
]);
const [listedDatabases, databasesFromPrivileges, databasesFromRoles] =
await Promise.all([
listDatabases(),
getDatabasesFromPrivileges(),
getDatabasesFromRoles(),
]);

const databases = uniqueBy(
// NB: Order is important, we want listed collections to take precedence
// if they were fetched successfully
[...databasesFromPrivileges, ...listedDatabases],
[...databasesFromRoles, ...databasesFromPrivileges, ...listedDatabases],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to make sense of the comment that was here before. Where's listed collections coming from?

Copy link
Collaborator

@mcasimir mcasimir Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm assuming this is a merge of databases taken from the privileges and the result of adminDb.command({listDatabases: ...}) + adminDb.command({listCollections: dbName}), probably the db objects returned by the commands are more accurate than the privileges one.

Databases taken from privileges are useful cause admins may assign privileges to non existing dbs or collections that would not show up otherwise.

'name'
).map((db) => {
return { _id: db.name, name: db.name, ...adaptDatabaseInfo(db) };
Expand Down
51 changes: 50 additions & 1 deletion packages/data-service/src/instance-detail-helper.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { ConnectionOptions } from './connection-options';
import type { InstanceDetails } from './instance-detail-helper';
import { checkIsCSFLEConnection } from './instance-detail-helper';
import {
getDatabasesByRoles,
getPrivilegesByDatabaseAndCollection,
getInstance,
} from './instance-detail-helper';
Expand Down Expand Up @@ -306,7 +307,55 @@ describe('instance-detail-helper', function () {
});
});

describe('#extractPrivilegesByDatabaseAndCollection', function () {
describe('#getDatabasesByRoles', function () {
it('returns a list of databases matching the roles', function () {
const dbs = getDatabasesByRoles(
[
{ db: 'not-test', role: 'write' },
{ db: 'test', role: 'read' },
{ db: 'pineapple', role: 'customRole123' },
{ db: 'pineapple', role: 'customRole12' },
{ db: 'theater', role: 'dbAdmin' },
],
['read', 'readWrite', 'dbAdmin', 'dbOwner']
);

expect(dbs).to.deep.eq(['test', 'theater']);
});

it('handles an empty list', function () {
const dbs = getDatabasesByRoles();

expect(dbs).to.deep.eq([]);
});

it('handles an empty list with roles', function () {
const dbs = getDatabasesByRoles(
[],
['read', 'readWrite', 'dbAdmin', 'dbOwner']
);

expect(dbs).to.deep.eq([]);
});

it('does not return a duplicate database entry', function () {
const dbs = getDatabasesByRoles(
[
{ db: 'test', role: 'read' },
{ db: 'pineapple', role: 'customRole123' },
{ db: 'pineapple', role: 'customRole12' },
{ db: 'theater', role: 'readWrite' },
{ db: 'theater', role: 'customRole1' },
{ db: 'test', role: 'readWrite' },
],
['read', 'readWrite', 'dbAdmin', 'dbOwner']
);

expect(dbs).to.deep.eq(['test', 'theater']);
});
});

describe('#getPrivilegesByDatabaseAndCollection', function () {
it('returns a tree of databases and collections from privileges', function () {
const dbs = getPrivilegesByDatabaseAndCollection([
{ resource: { db: 'foo', collection: 'bar' }, actions: [] },
Expand Down
25 changes: 25 additions & 0 deletions packages/data-service/src/instance-detail-helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,31 @@ export function getPrivilegesByDatabaseAndCollection(
return result;
}

// Return a list of the databases which have a role matching one of the roles.
export function getDatabasesByRoles(
authenticatedUserRoles:
| ConnectionStatusWithPrivileges['authInfo']['authenticatedUserRoles']
| null = null,
possibleRoles: string[] | null = null
): string[] {
const roles = authenticatedUserRoles ?? [];

const results = new Set<string>();

const filteredRoles =
possibleRoles && possibleRoles.length > 0
? roles.filter(({ role }) => {
return possibleRoles.includes(role);
})
: roles;

for (const { db } of filteredRoles) {
results.add(db);
}

return [...results];
}

function isNotAuthorized(err: AnyError) {
if (!err) {
return false;
Expand Down
1 change: 1 addition & 0 deletions packages/database-model/lib/model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ const DatabaseCollection = AmpersandCollection.extend(
const dbs = await dataService.listDatabases({
nameOnly: true,
privileges: instanceModel.auth.privileges,
roles: instanceModel.auth.roles
});

this.set(dbs.map(({ _id, name }) => ({ _id, name })));
Expand Down