SERVER-38867 refine handling of system collections in listCollections

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<std::string> 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<ResourcePattern, Privilege>& 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<std::string> 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<ResourcePattern, Privilege>& 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);
         }