Permalink
Browse files

SERVER-7767 Validate system.users documents on insert and update.

  • Loading branch information...
1 parent 6c71d93 commit e223993deff1dfd1f054acd01c29759b56a9afb1 @andy10gen andy10gen committed Dec 19, 2012
View
48 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());
View
3 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
View
2 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
View
8 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" );
View
12 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 ) {
View
8 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

0 comments on commit e223993

Please sign in to comment.