Merge branch 'accept_new_docs'

jstests/validate_user_documents.js

@@ -0,0 +1,48 @@
+// Ensure that inserts and updates of the system.users collection validate the schema of inserted
+// documents.
+
+function assertGLEOK(status) {
+    assert(status.ok && status.err === null,
+           "Expected OK status object; found " + tojson(status));
+}
+
+function assertGLENotOK(status) {
+    assert(status.ok && status.err !== null,
+           "Expected not-OK status object; found " + tojson(status));
+}
+
+db.dropDatabase();
+
+//
+// Tests of the insert path
+//
+
+// Valid compatibility document; insert should succeed.
+db.system.users.insert({ user: "spencer", pwd: hex_md5("spencer:mongo:a"), readOnly: true });
+assertGLEOK(db.getLastErrorObj());
+
+// Invalid compatibility document; insert should fail.
+db.system.users.insert({ user: "andy", readOnly: true });
+assertGLENotOK(db.getLastErrorObj());
+
+// Valid extended document; insert should succeed.
+db.system.users.insert({ user: "spencer", userSource: "test2", roles: ["dbAdmin"] });
+assertGLEOK(db.getLastErrorObj());
+
+// Invalid extended document; insert should fail.
+db.system.users.insert({ user: "andy", userSource: "test2", roles: ["dbAdmin", 15] });
+assertGLENotOK(db.getLastErrorObj());
+
+
+//
+// Tests of the update path
+//
+
+// Update a document in a legal way, expect success.
+db.system.users.update({user: "spencer", userSource: null}, { $set: {readOnly: false} });
+assertGLEOK(db.getLastErrorObj());
+
+
+// Update a document in a way that is illegal, expect failure.
+db.system.users.update({user: "spencer", userSource: null}, { $unset: {pwd: 1} });
+assertGLENotOK(db.getLastErrorObj());

src/mongo/client/redef_macros.h

@@ -48,6 +48,9 @@
 #pragma push_macro("uassert")
 #undef uassert
 #define uassert MONGO_uassert
+#pragma push_macro("uassertStatusOK")
+#undef uassertStatusOK
+#define uassertStatusOK MONGO_uassertStatusOK
 #pragma push_macro("DESTRUCTOR_GUARD")
 #undef DESTRUCTOR_GUARD
 #define DESTRUCTOR_GUARD MONGO_DESTRUCTOR_GUARD

src/mongo/client/undef_macros.h

@@ -38,6 +38,8 @@
 #pragma pop_macro("massert")
 #undef uassert
 #pragma pop_macro("uassert")
+#undef uassertStatusOK
+#pragma pop_macro("uassertStatusOK")
 #undef verify
 #pragma pop_macro("verify")
 #undef DESTRUCTOR_GUARD

src/mongo/db/auth/authorization_manager.cpp

@@ -29,6 +29,7 @@
 #include "mongo/db/auth/privilege.h"
 #include "mongo/db/auth/privilege_set.h"
 #include "mongo/db/client.h"
+#include "mongo/db/jsobj.h"
 #include "mongo/db/namespacestring.h"
 #include "mongo/db/security_common.h"
 #include "mongo/util/mongoutils/str.h"
@@ -216,6 +217,119 @@ namespace {
         return Status::OK();
     }
 
+    static inline Status _badValue(const char* reason, int location) {
+        return Status(ErrorCodes::BadValue, reason, location);
+    }
+
+    static inline Status _badValue(const std::string& reason, int location) {
+        return Status(ErrorCodes::BadValue, reason, location);
+    }
+
+    static inline StringData makeStringDataFromBSONElement(const BSONElement& element) {
+        return StringData(element.valuestr(), element.valuestrsize() - 1);
+    }
+
+    static Status _checkRolesArray(const BSONElement& rolesElement) {
+        if (rolesElement.type() != Array) {
+            return _badValue("Role fields must be an array when present in  system.users entries",
+                             0);
+        }
+        for (BSONObjIterator iter(rolesElement.embeddedObject()); iter.more(); iter.next()) {
+            BSONElement element = *iter;
+            if (element.type() != String || makeStringDataFromBSONElement(element).empty()) {
+                return _badValue("Roles must be non-empty strings.", 0);
+            }
+        }
+        return Status::OK();
+    }
+
+    Status AuthorizationManager::checkValidPrivilegeDocument(const StringData& dbname,
+                                                             const BSONObj& doc) {
+        BSONElement userElement = doc[USERNAME_FIELD_NAME];
+        BSONElement userSourceElement = doc[USERSOURCE_FIELD_NAME];
+        BSONElement passwordElement = doc[PASSWORD_FIELD_NAME];
+        BSONElement rolesElement = doc[ROLES_FIELD_NAME];
+        BSONElement otherDBRolesElement = doc[OTHER_DB_ROLES_FIELD_NAME];
+        BSONElement readOnlyElement = doc[READONLY_FIELD_NAME];
+
+        // Validate the "user" element.
+        if (userElement.type() != String)
+            return _badValue("system.users entry needs 'user' field to be a string", 14051);
+        if (makeStringDataFromBSONElement(userElement).empty())
+            return _badValue("system.users entry needs 'user' field to be non-empty", 14053);
+
+        // Must set exactly one of "userSource" and "pwd" fields.
+        if (userSourceElement.eoo() == passwordElement.eoo()) {
+            return _badValue("system.users entry must have either a 'pwd' field or a 'userSource' "
+                             "field, but not both", 0);
+        }
+
+        // Cannot have both "roles" and "readOnly" elements.
+        if (!rolesElement.eoo() && !readOnlyElement.eoo()) {
+            return _badValue("system.users entry must not have both 'roles' and 'readOnly' fields",
+                             0);
+        }
+
+        // Validate the "pwd" element, if present.
+        if (!passwordElement.eoo()) {
+            if (passwordElement.type() != String)
+                return _badValue("system.users entry needs 'pwd' field to be a string", 14052);
+            if (makeStringDataFromBSONElement(passwordElement).empty())
+                return _badValue("system.users entry needs 'pwd' field to be non-empty", 14054);
+        }
+
+        // Validate the "userSource" element, if present.
+        if (!userSourceElement.eoo()) {
+            if (userSourceElement.type() != String ||
+                makeStringDataFromBSONElement(userSourceElement).empty()) {
+
+                return _badValue("system.users entry needs 'userSource' field to be a non-empty "
+                                 "string, if present", 0);
+            }
+            if (userSourceElement.str() == dbname) {
+                return _badValue(mongoutils::str::stream() << "'" << dbname <<
+                                 "' is not a valid value for the userSource field in " <<
+                                 dbname << ".system.users entries",
+                                 0);
+            }
+            if (rolesElement.eoo()) {
+                return _badValue("system.users entry needs 'roles' field if 'userSource' field "
+                                 "is present.", 0);
+            }
+        }
+
+        // Validate the "roles" element.
+        if (!rolesElement.eoo()) {
+            Status status = _checkRolesArray(rolesElement);
+            if (!status.isOK())
+                return status;
+        }
+
+        if (!otherDBRolesElement.eoo()) {
+            if (dbname != ADMIN_DBNAME) {
+                return _badValue("Only admin.system.users entries may contain 'otherDBRoles' "
+                                 "fields", 0);
+            }
+            if (rolesElement.eoo()) {
+                return _badValue("system.users entries with 'otherDBRoles' fields must contain "
+                                 "'roles' fields", 0);
+            }
+            if (otherDBRolesElement.type() != Object) {
+                return _badValue("'otherDBRoles' field must be an object when present in "
+                                 "system.users entries", 0);
+            }
+            for (BSONObjIterator iter(otherDBRolesElement.embeddedObject());
+                 iter.more(); iter.next()) {
+
+                Status status = _checkRolesArray(*iter);
+                if (!status.isOK())
+                    return status;
+            }
+        }
+
+        return Status::OK();
+    }
+
     AuthorizationManager::AuthorizationManager(AuthExternalState* externalState) {
         _externalState.reset(externalState);
     }

src/mongo/db/auth/authorization_manager.h

@@ -59,6 +59,12 @@ namespace mongo {
         static const std::string SERVER_RESOURCE_NAME;
         static const std::string CLUSTER_RESOURCE_NAME;
 
+        // Checks to see if "doc" is a valid privilege document, assuming it is stored in the
+        // "system.users" collection of database "dbname".
+        //
+        // Returns Status::OK() if the document is good, or Status(ErrorCodes::BadValue), otherwise.
+        static Status checkValidPrivilegeDocument(const StringData& dbname, const BSONObj& doc);
+
         // Takes ownership of the externalState.
         explicit AuthorizationManager(AuthExternalState* externalState);
         ~AuthorizationManager();

src/mongo/db/auth/authorization_manager_test.cpp

@@ -371,5 +371,104 @@ namespace {
                               "anydb", principal, oldAndNewMixed, &result));
     }
 
+    TEST(AuthorizationManagerTest, DocumentValidationCompatibility) {
+        Status (*check)(const StringData&, const BSONObj&) =
+            &AuthorizationManager::checkValidPrivilegeDocument;
+
+        // Good documents, with and without "readOnly" fields.
+        ASSERT_OK(check("test", BSON("user" << "andy" << "pwd" << "a")));
+        ASSERT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" << "readOnly" << 1)));
+        ASSERT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" << "readOnly" << false)));
+        ASSERT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" << "readOnly" << "yes")));
+
+        // Must have a "pwd" field.
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy")));
+
+        // "pwd" field must be a string.
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy" << "pwd" << 100)));
+
+        // "pwd" field string must not be empty.
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy" << "pwd" << "")));
+
+        // Must have a "user" field.
+        ASSERT_NOT_OK(check("test", BSON("pwd" << "a")));
+
+        // "user" field must be a string.
+        ASSERT_NOT_OK(check("test", BSON("user" << 100 << "pwd" << "a")));
+
+        // "user" field string must not be empty.
+        ASSERT_NOT_OK(check("test", BSON("user" << "" << "pwd" << "a")));
+    }
+
+    TEST(AuthorizationManagerTest, DocumentValidationExtended) {
+        Status (*check)(const StringData&, const BSONObj&) =
+            &AuthorizationManager::checkValidPrivilegeDocument;
+
+        // Document describing new-style user on "test".
+        ASSERT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" <<
+                                     "roles" << BSON_ARRAY("read"))));
+
+        // Document giving roles on "test" to a user from "test2".
+        ASSERT_OK(check("test", BSON("user" << "andy" << "userSource" << "test2" <<
+                                     "roles" << BSON_ARRAY("read"))));
+
+        // Cannot have "userSource" field value == dbname.
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy" << "userSource" << "test" <<
+                                         "roles" << BSON_ARRAY("read"))));
+
+        // Cannot have both "userSource" and "pwd"
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy" << "userSource" << "test2" <<
+                                         "pwd" << "a" << "roles" << BSON_ARRAY("read"))));
+
+        // Cannot have an otherDBRoles field except in the admin database.
+        ASSERT_NOT_OK(check("test",
+                            BSON("user" << "andy" << "userSource" << "test2" <<
+                                 "roles" << BSON_ARRAY("read") <<
+                                 "otherDBRoles" << BSON("test2" << BSON_ARRAY("readWrite")))));
+
+        ASSERT_OK(check("admin",
+                        BSON("user" << "andy" << "userSource" << "test2" <<
+                             "roles" << BSON_ARRAY("read") <<
+                             "otherDBRoles" << BSON("test2" << BSON_ARRAY("readWrite")))));
+
+        // Must have "roles" to have "otherDBRoles".
+        ASSERT_NOT_OK(check("admin",
+                            BSON("user" << "andy" << "pwd" << "a" <<
+                                 "otherDBRoles" << BSON("test2" << BSON_ARRAY("readWrite")))));
+
+        ASSERT_OK(check("admin",
+                        BSON("user" << "andy" << "pwd" << "a" <<
+                             "roles" << BSONArrayBuilder().arr() <<
+                             "otherDBRoles" << BSON("test2" << BSON_ARRAY("readWrite")))));
+
+        // "otherDBRoles" may be empty.
+        ASSERT_OK(check("admin",
+                        BSON("user" << "andy" << "pwd" << "a" <<
+                             "roles" << BSONArrayBuilder().arr() <<
+                             "otherDBRoles" << BSONObjBuilder().obj())));
+
+        // Cannot omit "roles" if "userSource" is present.
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy" << "userSource" << "test2")));
+
+        // Cannot have both "roles" and "readOnly".
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" << "readOnly" << 1 <<
+                                         "roles" << BSON_ARRAY("read"))));
+
+        // Roles must be strings, not empty.
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" <<
+                                         "roles" << BSON_ARRAY("read" << ""))));
+
+        ASSERT_NOT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" <<
+                                         "roles" << BSON_ARRAY(1 << "read"))));
+
+        // Multiple roles OK.
+        ASSERT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" <<
+                                     "roles" << BSON_ARRAY("dbAdmin" << "read"))));
+
+        // Empty roles list OK.
+        ASSERT_OK(check("test", BSON("user" << "andy" << "pwd" << "a" <<
+                                     "roles" << BSONArrayBuilder().arr())));
+    }
+
 }  // namespace
 }  // namespace mongo

src/mongo/db/ops/update.cpp

@@ -356,7 +356,13 @@ namespace mongo {
                         c->prepareToTouchEarlierIterate();
                     }
 
-                    if ( modsIsIndexed <= 0 && mss->canApplyInPlace() ) {
+                    // If we've made it this far, "ns" must contain a valid collection name, and so
+                    // is of the form "db.collection".  Therefore, the following expression must
+                    // always be valid.  "system.users" updates must never be done in place, in
+                    // order to ensure that they are validated inside DataFileMgr::updateRecord(.).
+                    bool isSystemUsersMod = (NamespaceString(ns).coll == "system.users");
+
+                    if ( modsIsIndexed <= 0 && mss->canApplyInPlace() && !isSystemUsersMod ) {
                         mss->applyModsInPlace( true );// const_cast<BSONObj&>(onDisk) );
 
                         DEBUGUPDATE( "\t\t\t doing in place update" );

src/mongo/db/pdfile.cpp

@@ -32,6 +32,7 @@ _ disallow system* manipulations from the database.
 #include <list>
 
 #include "mongo/base/counter.h"
+#include "mongo/db/auth/authorization_manager.h"
 #include "mongo/db/pdfile_private.h"
 #include "mongo/db/background.h"
 #include "mongo/db/btree.h"
@@ -1131,6 +1132,11 @@ namespace mongo {
             objNew = b.obj();
         }
 
+        NamespaceString nsstring(ns);
+        if (nsstring.coll == "system.users") {
+            uassertStatusOK(AuthorizationManager::checkValidPrivilegeDocument(nsstring.db, objNew));
+        }
+
         /* duplicate key check. we descend the btree twice - once for this check, and once for the actual inserts, further
            below.  that is suboptimal, but it's pretty complicated to do it the other way without rollbacks...
         */
@@ -1369,10 +1375,8 @@ namespace mongo {
             else if ( legalClientSystemNS( ns , true ) ) {
                 if ( obuf && strstr( ns , ".system.users" ) ) {
                     BSONObj t( reinterpret_cast<const char *>( obuf ) );
-                    uassert( 14051 , "system.users entry needs 'user' field to be a string" , t["user"].type() == String );
-                    uassert( 14052 , "system.users entry needs 'pwd' field to be a string" , t["pwd"].type() == String );
-                    uassert( 14053 , "system.users entry needs 'user' field to be non-empty" , t["user"].String().size() );
-                    uassert( 14054 , "system.users entry needs 'pwd' field to be non-empty" , t["pwd"].String().size() );
+                    uassertStatusOK(AuthorizationManager::checkValidPrivilegeDocument(
+                                            nsToDatabaseSubstring(ns), t));
                 }
             }
             else if ( !god ) {

src/mongo/util/assert_util.h

@@ -186,6 +186,13 @@ namespace mongo {
     /* "user assert".  if asserts, user did something wrong, not our code */
 #define MONGO_uassert(msgid, msg, expr) (void)( MONGO_likely(!!(expr)) || (mongo::uasserted(msgid, msg), 0) )
 
+#define MONGO_uassertStatusOK(expr) do {                                  \
+        Status status = (expr);                                         \
+        if (!status.isOK())                                             \
+            uasserted(status.location() != 0 ? status.location() : status.code(), \
+                      status.reason());                                 \
+    } while(0)
+
     /* warning only - keeps going */
 #define MONGO_wassert(_Expression) (void)( MONGO_likely(!!(_Expression)) || (mongo::wasserted(#_Expression, __FILE__, __LINE__), 0) )
 
@@ -221,6 +228,7 @@ namespace mongo {
 # define dassert MONGO_dassert
 # define verify MONGO_verify
 # define uassert MONGO_uassert
+# define uassertStatusOK MONGO_uassertStatusOK
 # define wassert MONGO_wassert
 # define massert MONGO_massert
 #endif