From 89fb6dcc3e87fca01bbea2a7662d33f6f2c4702d Mon Sep 17 00:00:00 2001 From: Kevin Pulo Date: Mon, 7 Jan 2019 03:57:07 +0000 Subject: SERVER-38867 refine handling of system collections in listCollections --- jstests/auth/list_collections_own_collections.js | 203 ++++++++++++++--------- src/mongo/db/commands/list_collections.cpp | 10 +- src/mongo/s/commands/commands_public.cpp | 87 +++++----- 3 files changed, 174 insertions(+), 126 deletions(-) diff --git a/jstests/auth/list_collections_own_collections.js b/jstests/auth/list_collections_own_collections.js index 9759a78752e..82d17afc787 100644 --- a/jstests/auth/list_collections_own_collections.js +++ b/jstests/auth/list_collections_own_collections.js @@ -4,6 +4,13 @@ const dbName = "list_collections_own_collections"; + const nameSort = (a, b) => a.name > b.name; + const resFoo = {"name": "foo", "type": "collection"}; + const resBar = {"name": "bar", "type": "view"}; + const resOther = + [{"name": "otherCollection", "type": "collection"}, {"name": "otherView", "type": "view"}]; + const resSystemViews = {"name": "system.views", "type": "collection"}; + function runTestOnConnection(conn) { const admin = conn.getDB("admin"); const db = conn.getDB(dbName); @@ -11,63 +18,70 @@ assert.commandWorked(admin.runCommand({createUser: "root", pwd: "root", roles: ["root"]})); assert(admin.auth("root", "root")); - assert.commandWorked(admin.runCommand({ - createRole: "roleWithExactNamespacePrivileges", - roles: [], - privileges: [ - {resource: {db: dbName, collection: "foo"}, actions: ["createCollection"]}, - {resource: {db: dbName, collection: "bar"}, actions: ["createCollection"]} - ] - })); - - assert.commandWorked(admin.runCommand({ - createRole: "roleWithExactNamespaceAndSystemPrivileges", - roles: [], - privileges: [ - {resource: {db: dbName, collection: "foo"}, actions: ["createCollection"]}, - {resource: {db: dbName, collection: "bar"}, actions: ["createCollection"]}, - { - resource: {db: dbName, collection: "system.views"}, - actions: ["createCollection"] - } - ] - })); - - assert.commandWorked(admin.runCommand({ - createRole: "roleWithCollectionPrivileges", - roles: [], - privileges: [ - {resource: {db: "", collection: "foo"}, actions: ["createCollection"]}, - {resource: {db: "", collection: "bar"}, actions: ["createCollection"]} - ] - })); - - assert.commandWorked(admin.runCommand({ - createRole: "roleWithDatabasePrivileges", - roles: [], - privileges: [ - {resource: {db: dbName, collection: ""}, actions: ["createCollection"]}, - ] - })); - - assert.commandWorked(admin.runCommand({ - createRole: "roleWithAnyNormalResourcePrivileges", - roles: [], - privileges: [ - {resource: {db: "", collection: ""}, actions: ["createCollection"]}, - ] - })); - admin.logout(); - - function runTestOnRole(roleName) { - assert(admin.auth("root", "root")); - assert.commandWorked(db.dropDatabase()); + function createTestRoleAndUser(roleName, privs) { + assert.commandWorked( + admin.runCommand({createRole: roleName, roles: [], privileges: privs})); const userName = "user|" + roleName; assert.commandWorked(db.runCommand( {createUser: userName, pwd: "pwd", roles: [{role: roleName, db: "admin"}]})); - admin.logout(); + } + + createTestRoleAndUser("roleWithExactNamespacePrivileges", [ + {resource: {db: dbName, collection: "foo"}, actions: ["find"]}, + {resource: {db: dbName, collection: "bar"}, actions: ["find"]} + ]); + + createTestRoleAndUser("roleWithExactNamespaceAndSystemPrivileges", [ + {resource: {db: dbName, collection: "foo"}, actions: ["find"]}, + {resource: {db: dbName, collection: "bar"}, actions: ["find"]}, + {resource: {db: dbName, collection: "system.views"}, actions: ["find"]} + ]); + + createTestRoleAndUser("roleWithCollectionPrivileges", [ + {resource: {db: "", collection: "foo"}, actions: ["find"]}, + {resource: {db: "", collection: "bar"}, actions: ["find"]} + ]); + + createTestRoleAndUser("roleWithCollectionAndSystemPrivileges", [ + {resource: {db: "", collection: "foo"}, actions: ["find"]}, + {resource: {db: "", collection: "bar"}, actions: ["find"]}, + {resource: {db: "", collection: "system.views"}, actions: ["find"]} + ]); + + createTestRoleAndUser("roleWithDatabasePrivileges", [ + {resource: {db: dbName, collection: ""}, actions: ["find"]}, + ]); + + createTestRoleAndUser("roleWithDatabaseAndSystemPrivileges", [ + {resource: {db: dbName, collection: ""}, actions: ["find"]}, + {resource: {db: dbName, collection: "system.views"}, actions: ["find"]} + ]); + + createTestRoleAndUser("roleWithAnyNormalResourcePrivileges", [ + {resource: {db: "", collection: ""}, actions: ["find"]}, + ]); + + createTestRoleAndUser("roleWithAnyNormalResourceAndSystemPrivileges", [ + {resource: {db: "", collection: ""}, actions: ["find"]}, + {resource: {db: "", collection: "system.views"}, actions: ["find"]} + ]); + + // Create the collection and view used by the tests. + assert.commandWorked(db.dropDatabase()); + assert.commandWorked(db.createCollection("foo")); + assert.commandWorked(db.createView("bar", "foo", [])); + + // Create a collection and view that are never granted specific permissions, to ensure + // they're only returned by listCollections when the role has access to the whole db/server. + assert.commandWorked(db.createCollection("otherCollection")); + assert.commandWorked(db.createView("otherView", "otherCollection", [])); + + admin.logout(); + function runTestOnRole(roleName, expectedColls) { + jsTestLog(roleName); + const userName = "user|" + roleName; assert(db.auth(userName, "pwd")); let res; @@ -78,28 +92,10 @@ assert.commandFailed(res); res = db.runCommand({listCollections: 1, authorizedCollections: true}); assert.commandFailed(res); - res = db.runCommand({listCollections: 1, nameOnly: true, authorizedCollections: true}); - assert.commandWorked(res); - assert.eq(0, res.cursor.firstBatch.length); - - assert.commandWorked(db.createCollection("foo")); res = db.runCommand({listCollections: 1, nameOnly: true, authorizedCollections: true}); assert.commandWorked(res); - assert.eq(1, res.cursor.firstBatch.length); - assert.eq([{"name": "foo", "type": "collection"}], res.cursor.firstBatch); - - assert.commandWorked(db.createView("bar", "foo", [])); - - res = db.runCommand({listCollections: 1, nameOnly: true, authorizedCollections: true}); - assert.commandWorked(res); - assert.eq(2, res.cursor.firstBatch.length, tojson(res.cursor.firstBatch)); - const sortFun = function(a, b) { - return a.name > b.name; - }; - assert.eq([{"name": "bar", "type": "view"}, {"name": "foo", "type": "collection"}].sort( - sortFun), - res.cursor.firstBatch.sort(sortFun)); + assert.eq(expectedColls.sort(nameSort), res.cursor.firstBatch.sort(nameSort)); res = db.runCommand({ listCollections: 1, @@ -108,24 +104,64 @@ filter: {"name": "foo"} }); assert.commandWorked(res); - assert.eq(1, res.cursor.firstBatch.length); - assert.eq([{"name": "foo", "type": "collection"}], res.cursor.firstBatch); + assert.eq([resFoo], res.cursor.firstBatch); db.logout(); } - runTestOnRole("roleWithExactNamespacePrivileges"); - runTestOnRole("roleWithExactNamespaceAndSystemPrivileges"); - runTestOnRole("roleWithCollectionPrivileges"); - runTestOnRole("roleWithDatabasePrivileges"); - runTestOnRole("roleWithAnyNormalResourcePrivileges"); + runTestOnRole("roleWithExactNamespacePrivileges", [resFoo, resBar]); + runTestOnRole("roleWithExactNamespaceAndSystemPrivileges", + [resFoo, resBar, resSystemViews]); + + runTestOnRole("roleWithCollectionPrivileges", [resFoo, resBar]); + runTestOnRole("roleWithCollectionAndSystemPrivileges", [resFoo, resBar, resSystemViews]); + + runTestOnRole("roleWithDatabasePrivileges", [resFoo, resBar, ...resOther]); + runTestOnRole("roleWithDatabaseAndSystemPrivileges", + [resFoo, resBar, ...resOther, resSystemViews]); + + runTestOnRole("roleWithAnyNormalResourcePrivileges", [resFoo, resBar, ...resOther]); + runTestOnRole("roleWithAnyNormalResourceAndSystemPrivileges", + [resFoo, resBar, ...resOther, resSystemViews]); + } + + function runNoAuthTestOnConnection(conn) { + const admin = conn.getDB("admin"); + const db = conn.getDB(dbName); + + assert.commandWorked(db.dropDatabase()); + assert.commandWorked(db.createCollection("foo")); + assert.commandWorked(db.createView("bar", "foo", [])); + + var resFull = db.runCommand({listCollections: 1}); + assert.commandWorked(resFull); + var resAuthColls = db.runCommand({listCollections: 1, authorizedCollections: true}); + assert.commandWorked(resAuthColls); + assert.eq(resFull.cursor.firstBatch.sort(nameSort), + resAuthColls.cursor.firstBatch.sort(nameSort)); + + var resNameOnly = db.runCommand({listCollections: 1, nameOnly: true}); + assert.commandWorked(resNameOnly); + var resNameOnlyAuthColls = + db.runCommand({listCollections: 1, nameOnly: true, authorizedCollections: true}); + assert.commandWorked(resNameOnlyAuthColls); + assert.eq(resNameOnly.cursor.firstBatch.sort(nameSort), + resNameOnlyAuthColls.cursor.firstBatch.sort(nameSort)); + + var resWithFilter = db.runCommand({ + listCollections: 1, + nameOnly: true, + authorizedCollections: true, + filter: {"name": "foo"} + }); + assert.commandWorked(resWithFilter); + assert.eq([{"name": "foo", "type": "collection"}], resWithFilter.cursor.firstBatch); } const mongod = MongoRunner.runMongod({auth: ''}); runTestOnConnection(mongod); MongoRunner.stopMongod(mongod); - // TODO: Remove 'shardAsReplicaSet: false' when SERVER-32672 is fixed. const st = new ShardingTest({ shards: 1, mongos: 1, @@ -135,4 +171,13 @@ runTestOnConnection(st.s0); st.stop(); + const mongodNoAuth = MongoRunner.runMongod(); + runNoAuthTestOnConnection(mongodNoAuth); + MongoRunner.stopMongod(mongodNoAuth); + + const stNoAuth = + new ShardingTest({shards: 1, mongos: 1, config: 1, other: {shardAsReplicaSet: false}}); + runNoAuthTestOnConnection(stNoAuth.s0); + stNoAuth.stop(); + }()); diff --git a/src/mongo/db/commands/list_collections.cpp b/src/mongo/db/commands/list_collections.cpp index 3e715d3a69a..2e1c8fece35 100644 --- a/src/mongo/db/commands/list_collections.cpp +++ b/src/mongo/db/commands/list_collections.cpp @@ -305,9 +305,8 @@ public: // Only validate on a per-collection basis if the user requested // a list of authorized collections if (authorizedCollections && - (nss.coll().startsWith("system.") || - !as->isAuthorizedForAnyActionOnResource( - ResourcePattern::forExactNamespace(nss)))) { + (!as->isAuthorizedForAnyActionOnResource( + ResourcePattern::forExactNamespace(nss)))) { continue; } @@ -327,9 +326,8 @@ public: MODE_IS, [&](Collection* collection, CollectionCatalogEntry* catalogEntry) { if (authorizedCollections && - (collection->ns().coll().startsWith("system.") || - !as->isAuthorizedForAnyActionOnResource( - ResourcePattern::forExactNamespace(collection->ns())))) { + (!as->isAuthorizedForAnyActionOnResource( + ResourcePattern::forExactNamespace(collection->ns())))) { return true; } BSONObj collBson = buildCollectionBson( diff --git a/src/mongo/s/commands/commands_public.cpp b/src/mongo/s/commands/commands_public.cpp index 1f89e4e3f59..3a47c10867a 100644 --- a/src/mongo/s/commands/commands_public.cpp +++ b/src/mongo/s/commands/commands_public.cpp @@ -373,53 +373,56 @@ public: mutablebson::Element newFilterAnd = rewrittenCmdObj.makeElementArray("$and"); uassertStatusOK(newFilter.pushBack(newFilterAnd)); - // Append a rule to the $and, which rejects system collections. - mutablebson::Element systemCollectionsFilter = rewrittenCmdObj.makeElementObject( - "", BSON("name" << BSON("$regex" << BSONRegEx("^(?!system\\.)")))); - uassertStatusOK(newFilterAnd.pushBack(systemCollectionsFilter)); - - if (!authzSession->isAuthorizedForAnyActionOnResource( + mutablebson::Element newFilterOr = rewrittenCmdObj.makeElementArray("$or"); + mutablebson::Element newFilterOrObj = rewrittenCmdObj.makeElementObject(""); + uassertStatusOK(newFilterOrObj.pushBack(newFilterOr)); + uassertStatusOK(newFilterAnd.pushBack(newFilterOrObj)); + + // DB resource grants all non-system collections, so filter out system collections. + // This is done inside the $or, since some system collections might be granted specific + // privileges. + if (authzSession->isAuthorizedForAnyActionOnResource( ResourcePattern::forDatabaseName(dbName))) { - // We passed an auth check which said we might be able to render some collections, - // but it doesn't seem like we should render all of them. We must filter. - - // Compute the set of collection names which would be permissible to return. - std::set collectionNames; - for (UserNameIterator nameIter = authzSession->getAuthenticatedUserNames(); - nameIter.more(); - nameIter.next()) { - User* authUser = authzSession->lookupUser(*nameIter); - const User::ResourcePrivilegeMap& resourcePrivilegeMap = authUser->getPrivileges(); - for (const std::pair& resourcePrivilege : - resourcePrivilegeMap) { - const auto& resource = resourcePrivilege.first; - if (resource.isCollectionPattern() || (resource.isExactNamespacePattern() && - resource.databaseToMatch() == dbName)) { - collectionNames.emplace(resource.collectionToMatch().toString()); - } + mutablebson::Element systemCollectionsFilter = rewrittenCmdObj.makeElementObject( + "", BSON("name" << BSON("$regex" << BSONRegEx("^(?!system\\.)")))); + uassertStatusOK(newFilterOr.pushBack(systemCollectionsFilter)); + } + + // Compute the set of collection names which would be permissible to return. + std::set collectionNames; + for (UserNameIterator nameIter = authzSession->getAuthenticatedUserNames(); nameIter.more(); + nameIter.next()) { + User* authUser = authzSession->lookupUser(*nameIter); + const User::ResourcePrivilegeMap& resourcePrivilegeMap = authUser->getPrivileges(); + for (const std::pair& resourcePrivilege : + resourcePrivilegeMap) { + const auto& resource = resourcePrivilege.first; + if (resource.isCollectionPattern() || + (resource.isExactNamespacePattern() && resource.databaseToMatch() == dbName)) { + collectionNames.emplace(resource.collectionToMatch().toString()); } } + } - // Construct a new filter predicate which returns only collections we were found to - // have privileges for. - BSONObjBuilder predicateBuilder; - BSONObjBuilder nameBuilder(predicateBuilder.subobjStart("name")); - BSONArrayBuilder setBuilder(nameBuilder.subarrayStart("$in")); + // Construct a new filter predicate which returns only collections we were found to + // have privileges for. + BSONObjBuilder predicateBuilder; + BSONObjBuilder nameBuilder(predicateBuilder.subobjStart("name")); + BSONArrayBuilder setBuilder(nameBuilder.subarrayStart("$in")); - // Load the de-duplicated set into a BSON array - for (StringData collectionName : collectionNames) { - setBuilder << collectionName; - } - setBuilder.done(); - nameBuilder.done(); + // Load the de-duplicated set into a BSON array + for (StringData collectionName : collectionNames) { + setBuilder << collectionName; + } + setBuilder.done(); + nameBuilder.done(); - collectionFilter = predicateBuilder.obj(); + collectionFilter = predicateBuilder.obj(); - // Filter the results by our collection names. - mutablebson::Element newFilterAndIn = - rewrittenCmdObj.makeElementObject("", collectionFilter); - uassertStatusOK(newFilterAnd.pushBack(newFilterAndIn)); - } + // Filter the results by our collection names. + mutablebson::Element newFilterCollections = + rewrittenCmdObj.makeElementObject("", collectionFilter); + uassertStatusOK(newFilterOr.pushBack(newFilterCollections)); // If there was a pre-existing filter, compose it with our new one. if (oldFilter.ok()) { @@ -443,7 +446,9 @@ public: BSONObj newCmd = cmdObj; - if (newCmd["authorizedCollections"].trueValue()) { + AuthorizationSession* authzSession = AuthorizationSession::get(opCtx->getClient()); + if (authzSession->getAuthorizationManager().isAuthEnabled() && + newCmd["authorizedCollections"].trueValue()) { newCmd = rewriteCommandForListingOwnCollections(opCtx, dbName, cmdObj); } -- cgit v1.2.1